nuts-foundation / nuts-node

The reference implementation of the Nuts specification. A decentralized identity network based on the w3c ssi concepts with practical functionality for the healthcare domain.
https://nuts-foundation.gitbook.io
GNU General Public License v3.0
23 stars 15 forks source link

Are we using the context package correctly? #2497

Open gerardsn opened 1 year ago

gerardsn commented 1 year ago

We are approaching 100 context.TODO() and context.Background() in production code (so excluding tests).

Generally speaking a new context should only be created at the root of the system (not sure how this works with the "structs should not contain a context" rule). All other contexts are derived from the system context, or are request specific in API/RPC calls. The only exceptions are if it is the wrong context, or the context contains no request specific information. The latter should not be used since it is always defined as "contains no specific information yet".

The main reason I see for having so many new contexts is dropping the existing context somewhere in the call stack. One example is

f(ctx context.Context, ...) -> validator.VerifyVP(no context, ...) -> validator.verifyVP(no context, ...) -> vcr.Search(context.TODO(), ...)

vcr.Search creates a new context because validator.VerifyVP contains no context, but everything calling validator.VerifyVP has a context.

References on context https://google.github.io/styleguide/go/decisions#contexts https://github.com/golang/go/wiki/CodeReviewComments#contexts:

Contexts

Values of the context.Context type carry security credentials, tracing information, deadlines, and cancellation signals across API and process boundaries. Go programs pass Contexts explicitly along the entire function call chain from incoming RPCs and HTTP requests to outgoing requests.

Most functions that use a Context should accept it as their first parameter:

func F(ctx context.Context, /* other arguments */) {}

A function that is never request-specific may use context.Background(), but err on the side of passing a Context even if you think you don't need to. The default case is to pass a Context; only use context.Background() directly if you have a good reason why the alternative is a mistake.

Don't add a Context member to a struct type; instead add a ctx parameter to each method on that type that needs to pass it along. The one exception is for methods whose signature must match an interface in the standard library or in a third party library.

Don't create custom Context types or use interfaces other than Context in function signatures.

If you have application data to pass around, put it in a parameter, in the receiver, in globals, or, if it truly belongs there, in a Context value.

Contexts are immutable, so it's fine to pass the same ctx to multiple calls that share the same deadline, cancellation signal, credentials, parent trace, etc.

woutslakhorst commented 1 year ago

indeed, context has to be passed if timeouts and cancellation is required.