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.

Sign up for GitHub

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

Jump to bottom

Clarify the use of the passed-in context. #114

Closed
wants to merge 1 commit into from
Closed

Clarify the use of the passed-in context. #114

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion services/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ func (s State) String() string {
//
type Service interface {
// StartAsync starts Service asynchronously. Service must be in New State, otherwise error is returned.
// Context is used as a parent context for service own context.
// Context is used as a parent context for all of the service's own child operation contexts.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"All" - there is normally just one. Why do you think previous description wasn't clear?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From reading implementations of StartAsync (I can find two: services.Manager and services.BasicService), I have to agree with @pstibrany. AFAICS "child operation contexts" would only describe the services.Manager implementation, which just acts as a proxy to its managed Services. The current documentation describes the context usage well IMO.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From reading implementations of StartAsync (I can find two: services.Manager and services.BasicService)

Note that services.Manager does not implement Service interface.

Copy link
Collaborator

@aknuds1 aknuds1 Jan 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pstibrany ugh, you're right. It only implements part of it (at least the StartAsync method). This is one reason I wish Go had explicit implementations (as Rust traits do), would make reading code a lot easier :/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

firstly, "for service own context" is nongrammatical -- it should at least be "for the service's own context".

I lost a week because I was passing in a timeout context, thinking that it would be used for the startup phase only. I am trying to make it more clear that the context is persisted inside the service object and used repeatedly.

Copy link
Collaborator

@aknuds1 aknuds1 Jan 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree that the language could be improved, but the PR changes the meaning of the comment :) I think the following would be the right phrasing (I refer to ctx as the argument):

// ctx is used as parent context for the service's own context.

Maybe the usage of the service context should be documented better (to avoid the type of rabbit hole @ywwg descended into)?

Copy link
Member

@pstibrany pstibrany Jan 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed privately with @ywwg. Paraphrased:

There is only one method that starts the service, and it takes a context, that is then passed to the service.

Context is "not used repeatedly" but during a lifetime of the service. If context becomes "finished", service will stop. This is a property of BasicService implementation.

Let's fix the grammar part. Tying lifecycle of the service with cancellation of the context may be too strong requirement for generic Service contract.

StartAsync(ctx context.Context) error

// AwaitRunning waits until service gets into Running state.
Expand Down