Simplify graphsync cancel #229
Conversation
dirkmc
force-pushed
the
fix/simple-gs-cancel
branch
from
9fdf1b2
to
14a7086
Compare
| @@ -95,9 +95,9 @@ var ChannelEvents = fsm.Events{ | |||
| chst.AddLog("data transfer receive error: %s", chst.Message) | |||
| return nil | |||
| }), | |||
| fsm.Event(datatransfer.RequestTimedOut).FromAny().ToNoChange().Action(func(chst *internal.ChannelState, err error) error { | |||
| fsm.Event(datatransfer.RequestCancelled).FromAny().ToNoChange().Action(func(chst *internal.ChannelState, err error) error { | |||
There was a problem hiding this comment.
Since we are changing this state transition and RequestTimedOut is abandoned now, should we migrate all RequestTimedOut to RequestCancelled, so that they're not left in limbo? Or are these terminal states?
Alternatively, we could rename RequestTimedOut to RequestCancelled?
There was a problem hiding this comment.
RequestTimedOut & RequestCancelled are events, not states, so I don't think they will get recorded anywhere. I don't think we need to migrate anything.
| // Error returns are logged but otherwise have no effect | ||
| OnRequestTimedOut(chid ChannelID, err error) error | ||
| OnRequestCancelled(chid ChannelID, err error) error |
There was a problem hiding this comment.
Do we need to percolate this change up to go-fil-markets, or lotus?
There was a problem hiding this comment.
This interface is only used internally so I don't think we need to make changes anywhere else
transport/graphsync/graphsync.go
Outdated
| func (t *Transport) executeGsRequest(req *gsReq) { | ||
| defer req.onComplete() |
There was a problem hiding this comment.
I'm not an expert in go-data-transfer, but what replaces the onComplete call here?
There was a problem hiding this comment.
Now that graphsync.CancelRequest() waits until the event queue is drained, we don't need to detect complete manually so we no longer need this callback
There was a problem hiding this comment.
Actually it is safer to wait for both the events to be drained, and the channel returned from graphsync.Request() to be drained. So in the end I think it does make sense to keep the onComplete mechanism 👍
I've restored it now.
| @@ -981,7 +920,7 @@ func (c *dtChannel) gsDataRequestRcvd(gsKey graphsyncKey, hookActions graphsync. | |||
|
|
|||
| // Save a mapping from the graphsync key to the channel ID so that | |||
| // subsequent graphsync callbacks are associated with this channel | |||
| c.gsKey = gsKey | |||
| c.gsKey = &gsKey | |||
There was a problem hiding this comment.
We don't seem to have acquired the lock before writing 🤔
There was a problem hiding this comment.
The lock is acquired on line 515. The code in this file is super complicated to follow.
Codecov Report
@@ Coverage Diff @@
## master #229 +/- ##
==========================================
- Coverage 67.61% 67.26% -0.35%
==========================================
Files 24 24
Lines 3032 3040 +8
==========================================
- Hits 2050 2045 -5
- Misses 632 639 +7
- Partials 350 356 +6
Continue to review full report at Codecov.
|
Depends on ipfs/go-graphsync#188