Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make stop channel work properly for both maintainCerts and maintainOCSP #379

Merged
merged 1 commit into from
Dec 20, 2019
Merged

Conversation

tomokinat
Copy link
Contributor

Current implementation passes stop channel to both maintainCerts and maintainOCSP functions. It won't work as expected unless stop is buffered (e.g. make(chan struct{}, 2)) and the value is sent twice, which will result in a arcane parameter precondition of an exported function CertCache.Init.

This PR fixes that problem in another way. PoC

@tomokinat
Copy link
Contributor Author

@twifkak
Copy link
Member

twifkak commented Dec 19, 2019

Good find! I think a simpler solution may be possible, per https://blog.golang.org/pipelines#TOC_6%2e:

We need a way to tell an unknown and unbounded number of goroutines to stop sending their values downstream. In Go, we can do this by closing a channel, because a receive operation on a closed channel can always proceed immediately, yielding the element type's zero value.

So can you try changing the code in main() to close the channel instead of sending a value?

If that doesn't work, then please extract this channel-forking code into a separate function and leave a comment explaining it.

Sorry about the flake! Restarted the build.

@tomokinat
Copy link
Contributor Author

Yes it works :) Thanks for the suggestion.

Yet I still feel like "pass to me stop chan struct{} and close it if you want to stop me" isn't a good I/F.
How about returning a function, similar to https://golang.org/pkg/context/#WithCancel ?

code

@twifkak
Copy link
Member

twifkak commented Dec 20, 2019

I'm okay changing the interface. Rather than returning a func from Init(), what about adding func (this *CertCache) Stop()? Init() could store the channel(s) in the CertCache struct.

@tomokinat
Copy link
Contributor Author

I just realized that if CertCache.Init is assumed to be callable even after once stopped for re-starting, stop should be initialized in Init, not in New. Should it be?

@twifkak
Copy link
Member

twifkak commented Dec 20, 2019

No, I think we only designed Init to be called once, and to be the first call after New. Should I merge now, or do you want to make any other changes?

@tomokinat
Copy link
Contributor Author

No, now this should be ready to merge :)

@twifkak
Copy link
Member

twifkak commented Dec 20, 2019

Thanks! (On second thought, I guess make would make sense inside New, but it's no big deal either way.)

@twifkak twifkak merged commit 4d78e9e into ampproject:master Dec 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants