ipfs / go-graphsync

Initial Implementation Of GraphSync Wire Protocol
Other
100 stars 38 forks source link

Fix potential lock on context cancel #391

Closed hannahhoward closed 2 years ago

hannahhoward commented 2 years ago

Goals

Fix an identified potential lock on imperative public function calls that take a context that can be cancelled.

Why

The graphsync RequestManager and ResponseManager run an internal run loop in a seperate go-routine. Public function calls access communicate with this run loop via a message queue channel. Sometimes these public function calls wait for a response on a channel, in order to return a value to the caller. When this is done, the message send into the run loop can abort if the calling context passed to the public method is cancelled. However, we were failing to account for this context when reading back out the response, which could cause a hang if the context passed into the public function is already cancelled at the time of calling. We found this did crop up in the wild.

Implementation

Add a calling context cancel check on the select statements in every place where we read a return value

For discussion

Contexts and channels suck and boy this code is verbose when can we use generics.

This is PR'd against a 0.13.3 release branch forked from v0.13.2 to avoid 0.14.x changes that are for go-data-transfer v2