Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better reconnect behaviour #162

Merged
merged 1 commit into from
Mar 18, 2021
Merged

Better reconnect behaviour #162

merged 1 commit into from
Mar 18, 2021

Conversation

dirkmc
Copy link
Contributor

@dirkmc dirkmc commented Mar 17, 2021

Fixes #160 #154 #156

@codecov-io
Copy link

Codecov Report

Merging #162 (afc4fb9) into master (42e0a5b) will increase coverage by 0.03%.
The diff coverage is 84.24%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #162      +/-   ##
==========================================
+ Coverage   65.78%   65.82%   +0.03%     
==========================================
  Files          23       23              
  Lines        2455     2446       -9     
==========================================
- Hits         1615     1610       -5     
  Misses        506      506              
+ Partials      334      330       -4     
Impacted Files Coverage Δ
impl/environment.go 75.00% <ø> (-10.72%) ⬇️
channels/channels.go 71.89% <33.33%> (-0.96%) ⬇️
transport/graphsync/graphsync.go 76.88% <35.71%> (-1.31%) ⬇️
channels/channels_fsm.go 68.75% <42.85%> (-9.83%) ⬇️
impl/impl.go 61.60% <54.54%> (+2.05%) ⬆️
channelmonitor/channelmonitor.go 83.25% <97.05%> (ø)
impl/events.go 72.32% <100.00%> (-1.72%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 42e0a5b...afc4fb9. Read the comment docs.

@dirkmc dirkmc marked this pull request as ready for review March 17, 2021 10:57
@dirkmc dirkmc requested a review from nonsense March 17, 2021 10:57
@@ -30,9 +30,3 @@ const ErrRejected = errorType("response rejected")

// ErrUnsupported indicates an operation is not supported by the transport protocol
const ErrUnsupported = errorType("unsupported")

// ErrDisconnected indicates the other peer may have hung up and you should try restarting the channel.
const ErrDisconnected = errorType("other peer appears to have hung up. restart Channel")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ErrDisconnected and ErrRemoved are no longer used (with this PR we try to reconnect automatically)

@@ -36,19 +34,6 @@ func (m *manager) OnDataReceived(chid datatransfer.ChannelID, link ipld.Link, si
return err
}

m.reconnectsLk.RLock()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove code that watches for the connection going down and coming up again, this is now handled by the channel monitor

@@ -85,75 +85,6 @@ func TestDataTransferInitiating(t *testing.T) {
testutil.AssertFakeDTVoucher(t, receivedRequest, h.voucher)
},
},
"Remove Timed-out request": {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests were for connection monitoring, which has now been removed in favour of the push / pull channel monitors

t.dataLock.Lock()
defer t.dataLock.Unlock()

chid, ok := t.graphsyncRequestMap[graphsyncKey{request.ID(), p}]
if ok {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Firing a Disconnect event here would cause go-fil-markets to fail the deal.
Instead we fire a SendDataError which the push / pull monitor listen for - they will attempt to reconnect and restart the channel

@dirkmc dirkmc merged commit c875cbe into master Mar 18, 2021
@dirkmc dirkmc deleted the feat/reconnect branch March 18, 2021 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor channel reconnect behaviour
3 participants