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

Remove context from task struct as it violates the pattern #31

Closed
nitishm opened this issue Feb 15, 2019 · 2 comments
Closed

Remove context from task struct as it violates the pattern #31

nitishm opened this issue Feb 15, 2019 · 2 comments
Labels
blocked Blocked until more information is added. bug Something isn't working Difficulty: Intermediate

Comments

@nitishm
Copy link
Owner

nitishm commented Feb 15, 2019

Describe the bug
Context should not be stored in structs, but instead be passed explicitly to the function. The current implementation violates the pattern and needs to be resolved.

To Reproduce
N/A

Expected behavior
N/A

Screenshots
N/A

Additional context
https://golang.org/pkg/context/ specifications explicitly state, not storing the context in the struct. In task.go we store the context and cancelFn within the struct. This was needed to provide a way to cancel running tasks, when the task.Cancel() function is invoked by the caller (i.e. the dispatcher, on handling a POST /v1/api/attack/<attackID/cancel request.

Potential Solution
The dispatcher is responsible for creating a new context for every task, prior to calling the tasks Run() method. The ctx should be passed to the Run() method explicitly, which can then be passed around among the task methods. The dispatcher instead can keep a reference to the context and invoke the cancel function by passing the stored context to the task's Cancel() method.

@nitishm
Copy link
Owner Author

nitishm commented Feb 15, 2019

The more I read on this, it seems like an accepted pattern, to store context's in the task struct. 🤔

References:
https://medium.com/@cep21/how-to-correctly-use-context-context-in-go-1-7-8f2c0fafdf39
golang/go#22602

In Dave Cheney https://dave.cheney.net/2017/08/20/context-isnt-for-cancellation states (ignoring the fact that he also expresses an opinion, that I agree with, that contexts should not be used for lifetime management):

Specifically context.Context values should only live in function arguments, never stored in a field or global. This makes context.Context applicable only to the lifetime of resources in a request’s scope. Given Go’s lineage on the server, this is a compelling use case. However, there exist other use cases for cancellation where the lifetime of the resource extends beyond a single request. For example, a background goroutine as part of an agent or pipeline.

Maybe a better solution in our case is leave it the way it is for now and re-visit this if needed.

@nitishm nitishm added blocked Blocked until more information is added. and removed up-for-grabs labels Feb 15, 2019
@nitishm
Copy link
Owner Author

nitishm commented Feb 22, 2019

No longer needed with fix #29

@nitishm nitishm closed this as completed Feb 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Blocked until more information is added. bug Something isn't working Difficulty: Intermediate
Projects
None yet
Development

No branches or pull requests

1 participant