Restart Push integration tests and corresponding fixes #78
Conversation
aarshkshah1992
changed the base branch from
master
to
feat/init-respond-unit-tests
aarshkshah1992
changed the title
| @@ -152,6 +152,13 @@ func (m *manager) OnResponseReceived(chid datatransfer.ChannelID, response datat | |||
| return m.resumeOther(chid) | |||
| } | |||
|
|
|||
| func (m *manager) OnRequestTimedOut(chid datatransfer.ChannelID) error { | |||
| // TODO Start a timer to cleanup state if this dosen't complete in a while | |||
There was a problem hiding this comment.
@hannahhoward Have created issue filecoin-project/lotus#3878 to clean these up from the FSM.
aarshkshah1992
changed the title
There was a problem hiding this comment.
All my comments are non-blocking except the naming of ManagerPeer, and we can put that in a seperate PR.
Am I correct that you're not disassembling the old data transfer instance in tests -- you just disconnect the peers, then reconnect and setup a new DT instance one of them? I guess that makes sense, since we don't really care what happens to it.
| @@ -15,6 +15,8 @@ import ( | |||
|
|
|||
| // channelState is immutable channel data plus mutable state | |||
| type channelState struct { | |||
| // peerId of the manager peer | |||
There was a problem hiding this comment.
this is an interesting change and probably useful.
I dunno if you saw but there's a method called OtherParty that takes a peer.ID for the party the data transfer is running on.
For symmetry, I'd like to do two things:
I'd like to rename ManagerPeer to ThisParty or SelfParty
and then I'd like to remove the parameter to OtherParty since we now have it in the channel state.
I guess I'd also be fine with SelfPeer and OtherPeer if Party doesn't make sense.
I'm ok with doing this as a seperate cleanup PR btw as well.
There was a problem hiding this comment.
This is done.
| @@ -29,5 +29,6 @@ require ( | |||
| github.com/libp2p/go-libp2p-core v0.5.0 | |||
| github.com/stretchr/testify v1.5.1 | |||
| github.com/whyrusleeping/cbor-gen v0.0.0-20200810223238-211df3b9e24c | |||
| go.uber.org/atomic v1.6.0 | |||
There was a problem hiding this comment.
did you mean to add this? I don't have an objection. I haven't worked with this library before but I'm sure I can figure it out.
There was a problem hiding this comment.
Yeah, it's a simple wrapper over "sync/atomic.
|
|
||
| waitCtx, cancel := context.WithTimeout(rh.testCtx, wait) | ||
| defer cancel() | ||
| i := 0 |
There was a problem hiding this comment.
tad silly but is this variable actually used?
There was a problem hiding this comment.
Great catch, not used anywhere. Have removed it.
| t.dataLock.Lock() | ||
| // if we have an existing request pending for the channelID, cancel it first. | ||
| if cancelF, ok := t.contextCancelMap[channelID]; ok { |
There was a problem hiding this comment.
non-blocking, something to consider: we may need to add a go routine that monitors transfers and cancels context for inactivity. reason being setting an overall timeout in go-fil-markets may not be a good sign of failure -- a large piece may just take a very long time. The real measure is transfer inactivity. anyway, let's cross that bridge later.
There was a problem hiding this comment.
I've added this note to filecoin-project/lotus#3878, an issue I've created to track cleanups.
aarshkshah1992
deleted the
feat/restart-manager-create-push-integration-tests
branch
|
@hannahhoward Addressed review in #79 |
For filecoin-project/lotus#3417.
Depends on
go-graphsyncPR ipfs/go-graphsync#93.Restart test is in
restart_integration_test.go