ipfs / go-graphsync

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

ResponseAssembler should run transaction even when function errors #261

Closed hannahhoward closed 2 years ago

hannahhoward commented 2 years ago

When I first wrote the ResponseAssembler, I thought of the "Transaction" method like a database transaction -- if it errors, you roll back. Hence these three lines:

https://github.com/ipfs/go-graphsync/blob/main/responsemanager/responseassembler/responseassembler.go#L101-L103

However, as it turns out, we ALWAYS end up wanting to execute the network operations in practice. As a result, we've ended up with awkward code throughout the response manager, because we want to make the network operations proceed, so we always make our transaction method return nil, and then attempt to capture the error, if we need it, via a closure. See this for example:

https://github.com/ipfs/go-graphsync/blob/main/responsemanager/queryexecutor.go#L96-L118

Which tends to provoke the response: but why are you returning nil when you have an error? (here for example: https://github.com/ipfs/go-graphsync/blob/main/responsemanager/queryexecutor.go#L100)

Since we are NEVER using the rollback functionality, remove conditional around rb.execute above.

Then go back through our code for transactions. We can now return an error inside the transaction, and rather than using a closure to capture it, simply assign the external error to the return value for .Transaction. This should allow us to simply and clarify our logic significantly.

rvagg commented 2 years ago

Pretty sure this is done as of #244 getting merged, awkward error capturing has been factored out and conditional execution has been removed