This repository has been archived by the owner on Feb 1, 2023. It is now read-only.
fix(wantlist): remove races on setup #72
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
assigned 
|
Jenkins seems to be out of space, hence broken builds |
Stebalien
reviewed
Feb 20, 2019
hannahhoward
commented
Feb 20, 2019
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I realize this somewhat reimplements sync.Map, but I find sync.Map relatively useless due to the lack of typing on the values (which can't be avoided given Go's lack of Generics at the moment)
Stebalien
reviewed
Feb 20, 2019
|
|
||
| // new peer, we will want to give them our full wantlist | ||
| // AddWantlist adds a complete session tracked want list to a message queue | ||
| func (mq *MessageQueue) AddWantlist(initialEntries []*wantlist.Entry) { |
There was a problem hiding this comment.
TODO (maybe later): It would be kind of nice to deduplicate this with AddMessage. As far as I can tell, they do approximately the same thing; they just take different inputs.
peermanager/peermanager.go
Outdated
| message.handle(pm) | ||
| case <-pm.ctx.Done(): | ||
| return | ||
| if ok { |
There was a problem hiding this comment.
Are Disconnect/Connect really safe? pm.get takes a lock internally but then we modify stuff without a lock. That seems a bit odd.
There was a problem hiding this comment.
I mean you're potentially mutating the object, but not its reference. The object itself is responsible for keeping internal mutations threadsafe. THOUGH I acknowledge that actually currently the refcount stuff is not thread safe. I think that should be handled inside the object though.
There was a problem hiding this comment.
I'm not sure about remove I need to double check.
peermanager/peermanager.go
Outdated
| log.Infof("tried sending wantlist change to non-partner peer: %s", t) | ||
| continue | ||
| p = pm.createPeerQueue(t) | ||
| pm.set(t, p) |
There was a problem hiding this comment.
Can nothing call this concurrently? If so, I assume we'd need some kind of atomic GetOrCreate kind of thing.
There was a problem hiding this comment.
mmm. good point. I get it.
|
@Stebalien I think I fixed the locking pattern, but feel free to double check. Also builds seem stalled for linux. |
|
When rebased on #76 and tested against the race detector, I'm getting the following error. It looks like |
|
Diff to fix the current race: Other than that, the race detector isn't agreeing with my spidy senses. |
fix race conditions while setting up wantlists by creating peer queues on demand BREAKING CHANGE: PeerManager SendMessage signature changed fix #51
Limit retrying sending of a message even when a successful reconnect occurs
If wantlist changes are present, send them immediately on startup, rather than as a seperate message
Breakup Startup function so that wantlists are not sent with each call to SendMessage
Constrain use of mutex to actual operations on the peerQueues map via utility functions
repace get/set with getOrCreate to keep operations atomic
Keep all of disconnection in a mutex
fix remaining issues for race detector in peer manager
hannahhoward
force-pushed
the
bugs/racy-wantlist-handling
branch
from
8dd1682
to
434e0f4
Compare
Move refcnt tracking from the messagequeue to the peermanager, where it's relevant
hannahhoward
force-pushed
the
bugs/racy-wantlist-handling
branch
from
055f143
to
d4191c4
Compare
Stebalien
approved these changes
Feb 20, 2019
Jorropo
pushed a commit
to Jorropo/go-libipfs
that referenced
this pull request
Jan 26, 2023
…ndling fix(wantlist): remove races on setup This commit was moved from ipfs/go-bitswap@472a8ab
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Goals
fix race conditions while setting up wantlists
Implementation
This change essentially solves bugs in initial wantlist handling using the strategy outlined by @Stebalien in #51.
This means two differences:
Other notable changes:
For Discussion
fix #51 fix #48