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

add Handle() callback to support Service-specific signal handling #28

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mreiferson
Copy link

This does what I said I was going to do in nsqio/nsq#1331

svc_common.go Outdated Show resolved Hide resolved
service.Context() is called after Init() (and after Start()) on unixes,
so some users did not expect or test with Context() called before Init()
@mreiferson
Copy link
Author

mreiferson commented Aug 15, 2021

I've pulled in #29 here so I can more easily get a build going

@judwhite
Copy link
Owner

@mreiferson I usually provide a package level function such as IsErrStop to check error types. It mimics how os handles errors, but it's different than io.EOF like you have here.

I like that the IsErrXXX way takes some of the guess work out, such as the caller asking "Is this the right way to check this error? Can I depend on the author considering errors as part of their API contract?" The Go team is explicit about io.EOF not changing.

I don't have a hard stance for public projects, especially since we don't have to worry about unwrapping. What are your thoughts? I'm open.

@mreiferson
Copy link
Author

I usually provide a package level function such as IsErrStop to check error types. It mimics how os handles errors, but it's different than io.EOF like you have here.

@judwhite I'm not sure that cleans things up in this case? The caller (Service provider) needs to return a specific error type to go-svc, not check an error type go-svc returns.

@ploxiln
Copy link

ploxiln commented Aug 16, 2021

Yeah, this is the opposite direction of the usual pattern.

This error returned by the user's callback to this library, is really a kind of sentinel value. It could alternatively be a boolean, or an enum of some kind ...

side note: the commit at the head of this branch is now referenced by https://github.com/nsqio/nsq/blob/v1.2.1/go.mod#L20 so we should theoretically be sure to keep this commit alive in mreiferson's fork indefinitely, maybe on another new branch so that this one can have requested changes rebased/squashed before merge

@mreiferson
Copy link
Author

Yep, pushed to https://github.com/mreiferson/go-svc/tree/nsq-v1.2.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants