nitishm / vegeta-server

A RESTful API server for vegeta, a load testing tool written in Go.
MIT License
63 stars 17 forks source link

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

Closed nitishm closed 5 years ago

nitishm commented 5 years ago

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 commented 5 years ago

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 https://github.com/golang/go/issues/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 commented 5 years ago

No longer needed with fix #29