go-livepeer: incorrect context handling

[mk-livepeer]

There are many calls to context.Background() across the go-livepeer codebase. This is wrong. Instead, ctx := context.Background() ...should be assigned once at the top of the main function, and

ctx, cancel := context.WithCancel(ctx)

...should be used to fork the context as necessary.

This covers the main() function, but all the others will also need to be modified to take a forked context out of main, rather than creating a new context.Background()

A forked context from ctx, or ctx itself should be passed from main() to the downstream functions:

cmd/livepeer/livepeer.go:       chainID, err := backend.ChainID(context.Background())
cmd/livepeer/livepeer.go:           ctx, cancel := context.WithCancel(context.Background())
cmd/livepeer/livepeer.go:           gasPriceUpdate, err := gpm.Start(context.Background())
cmd/livepeer/livepeer.go:           rs.Start(context.Background())
cmd/livepeer/livepeer.go:       blockWatchCtx, cancel := context.WithCancel(context.Background())
cmd/livepeer/livepeer.go:           ctx, cancel := context.WithCancel(context.Background())
cmd/livepeer/livepeer.go:   msCtx, cancel := context.WithCancel(context.Background())
common/testutil.go: return peer.NewContext(context.Background(), p)
core/orchestrator.go:   return context.WithTimeout(context.Background(), transcodeLoopTimeout)
core/orchestrator.go:           ctx, cancel := context.WithTimeout(context.Background(), transcodeLoopTimeout)
core/orchestrator.go:   ctx, cancel := context.WithTimeout(context.Background(), RemoteTranscoderTimeout)
discovery/db_discovery.go:  ctx, cancel := context.WithTimeout(context.Background(), getOrchestratorsTimeoutLoop)
discovery/db_discovery.go:  ctx, cancel := context.WithTimeout(context.Background(), getOrchestratorsTimeoutLoop)
discovery/discovery.go: ctx, cancel := context.WithTimeout(context.Background(), getOrchestratorsTimeoutLoop)
eth/blockwatch/client.go:   ctx, cancel := context.WithTimeout(context.Background(), rc.requestTimeout)
eth/blockwatch/client.go:   ctx, cancel := context.WithTimeout(context.Background(), rc.requestTimeout)
eth/blockwatch/client.go:   ctx, cancel := context.WithTimeout(context.Background(), rc.requestTimeout)
eth/client.go:  chainID, err := eth.ChainID(context.Background())
eth/client.go:  ctx, cancel := context.WithTimeout(context.Background(), c.txTimeout)
eth/client.go:  _, pending, err := c.backend.TransactionByHash(context.Background(), tx.Hash())
eth/client.go:      suggestedGasPrice, err := c.backend.SuggestGasPrice(context.Background())
eth/client.go:  err = c.backend.SendTransaction(context.Background(), newSignedTx)
eth/eventservices/rewardservice.go: cancelCtx, cancel := context.WithCancel(context.Background())
eth/noncemanager.go:    remoteNonce, err := m.remoteReader.PendingNonceAt(context.Background(), addr)
monitor/census.go:  census.ctx, err = tag.New(context.Background(), tag.Insert(census.kNodeType, nodeType), tag.Insert(census.kNodeID, nodeID))
monitor/census.go:  ctx, err := tag.New(context.Background(), tag.Insert(census.kNodeType, nodeType), tag.Insert(census.kNodeID, nodeID),
server/mediaserver.go:  lpmsCtx, cancel := context.WithCancel(context.Background())
server/mediaserver.go:          err := s.RTMPSegmenter.SegmentRTMPToHLS(context.Background(), rtmpStrm, hlsStrm, segOptions)
server/ot_rpc.go:   ctx := context.Background()
server/rpc.go:  ctx, cancel := context.WithTimeout(context.Background(), GRPCTimeout)
server/webserver.go:            balance, err := b.BalanceAt(context.Background(), s.LivepeerNode.Eth.Account().Address, nil)
server/webserver.go:            blk, err := backend.BlockByNumber(context.Background(), nil)
server/webserver.go:        chainID, err := be.ChainID(context.Background())

[mkrufky]

Context should flow through your program

A great mental model of using Context is that it should flow through your program. Imagine a river or running water. This generally means that you don’t want to store it somewhere like in a struct. Nor do you want to keep it around any more than strictly needed. Context should be an interface that is passed from function to function down your call stack, augmented as needed.

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

[j0sh]

There is certainly room for us to improve how we use contexts. For example, perhaps to signal shutdown rather than use ad-hoc channels for certain goroutines.

My general feeling about contexts in golang is that they are a bit slapdash. The "best practice" of not storing the context means anywhere a context is used downstream, it infects function signatures until there are contexts being passed around everywhere. It very much reminds me of the "red vs blue function" analogy - https://journal.stuffwithstuff.com/2015/02/01/what-color-is-your-function/

I suspect we use context.Background so often because of downstream dependencies that expect a context, but we don't otherwise have a pressing need to cancel anything other than maybe a timer. It might be nice to actually remove their usage where possible (eg, for our code takes a context but otherwise doesn't do anything with it).

If we choose to propagate a single cancellable context everywhere, we would need to take a closer look at how upstream context termination may affect the code in question. I'm concerned that changing all this in one fell swoop is less of a mechanical change than it seems.