Skip to content
This repository has been archived by the owner on Feb 1, 2023. It is now read-only.

refactor: simplify messageQueue onSent #349

Merged
merged 4 commits into from
Apr 13, 2020
Merged

refactor: simplify messageQueue onSent #349

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e06ac24
refactor: simplify messageQueue onSent
dirkmc Apr 13, 2020
efd006e
refactor: save some vars
dirkmc Apr 13, 2020
6c41260
refactor: remove unnecessary func param
dirkmc Apr 13, 2020
b6a8a73
fix: only mark sent wants as sent
dirkmc Apr 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 16 additions & 28 deletions internal/messagequeue/messagequeue.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,8 @@ func (r *recallWantlist) RemoveType(c cid.Cid, wtype pb.Message_Wantlist_WantTyp
r.pending.RemoveType(c, wtype)
}

// Sent moves the want from the pending to the sent list
func (r *recallWantlist) Sent(e bsmsg.Entry) {
// MarkSent moves the want from the pending to the sent list
func (r *recallWantlist) MarkSent(e wantlist.Entry) {
r.pending.RemoveType(e.Cid, e.WantType)
r.sent.Add(e.Cid, e.Priority, e.WantType)
}
Expand Down Expand Up @@ -439,7 +439,7 @@ func (mq *MessageQueue) sendMessage() {
for i := 0; i < maxRetries; i++ {
if mq.attemptSendAndRecovery(message) {
// We were able to send successfully.
onSent(wantlist)
onSent()

mq.simulateDontHaveWithTimeout(wantlist)

Expand Down Expand Up @@ -540,7 +540,7 @@ func (mq *MessageQueue) pendingWorkCount() int {
}

// Convert the lists of wants into a Bitswap message
func (mq *MessageQueue) extractOutgoingMessage(supportsHave bool) (bsmsg.BitSwapMessage, func([]bsmsg.Entry)) {
func (mq *MessageQueue) extractOutgoingMessage(supportsHave bool) (bsmsg.BitSwapMessage, func()) {
mq.wllock.Lock()
defer mq.wllock.Unlock()

Expand All @@ -566,8 +566,9 @@ func (mq *MessageQueue) extractOutgoingMessage(supportsHave bool) (bsmsg.BitSwap
}

// Add each regular want-have / want-block to the message
for i := 0; i < len(peerEntries) && msgSize < mq.maxMessageSize; i++ {
e := peerEntries[i]
peerSentCount := 0
for ; peerSentCount < len(peerEntries) && msgSize < mq.maxMessageSize; peerSentCount++ {
e := peerEntries[peerSentCount]
// If the remote peer doesn't support HAVE / DONT_HAVE messages,
// don't send want-haves (only send want-blocks)
if !supportsHave && e.WantType == pb.Message_Wantlist_Have {
Expand All @@ -578,7 +579,8 @@ func (mq *MessageQueue) extractOutgoingMessage(supportsHave bool) (bsmsg.BitSwap
}

// Add each broadcast want-have to the message
for i := 0; i < len(bcstEntries) && msgSize < mq.maxMessageSize; i++ {
bcstSentCount := 0
for ; bcstSentCount < len(bcstEntries) && msgSize < mq.maxMessageSize; bcstSentCount++ {
// Broadcast wants are sent as want-have
wantType := pb.Message_Wantlist_Have

Expand All @@ -588,41 +590,27 @@ func (mq *MessageQueue) extractOutgoingMessage(supportsHave bool) (bsmsg.BitSwap
wantType = pb.Message_Wantlist_Block
}

e := bcstEntries[i]
e := bcstEntries[bcstSentCount]
msgSize += mq.msg.AddEntry(e.Cid, e.Priority, wantType, false)
}

// Called when the message has been successfully sent.
onMessageSent := func(wantlist []bsmsg.Entry) {
bcst := keysToSet(bcstEntries)
prws := keysToSet(peerEntries)

onMessageSent := func() {
mq.wllock.Lock()
defer mq.wllock.Unlock()

// Move the keys from pending to sent
for _, e := range wantlist {
if _, ok := bcst[e.Cid]; ok {
mq.bcstWants.Sent(e)
}
if _, ok := prws[e.Cid]; ok {
mq.peerWants.Sent(e)
}
for i := 0; i < bcstSentCount; i++ {
mq.bcstWants.MarkSent(bcstEntries[i])
}
for i := 0; i < peerSentCount; i++ {
mq.peerWants.MarkSent(peerEntries[i])
Copy link
Member

Choose a reason for hiding this comment

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

This won't quite work as we will have removed some of the peer wants to peers that don't support HAVE.

Copy link
Contributor Author

@dirkmc dirkmc Apr 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

We remove them when we realize the peer doesn't support HAVE, so in MarkSent() we could just check if they've already been removed from pending before adding them to sent.
But actually I think it's clearer to put them in a list. I pushed a commit with this change.

}
}

return mq.msg, onMessageSent
}

// Convert wantlist entries into a set of cids
func keysToSet(wl []wantlist.Entry) map[cid.Cid]struct{} {
set := make(map[cid.Cid]struct{}, len(wl))
for _, e := range wl {
set[e.Cid] = struct{}{}
}
return set
}

func (mq *MessageQueue) initializeSender() error {
if mq.sender != nil {
return nil
Expand Down