Skip to content
This repository has been archived by the owner on Aug 2, 2021. It is now read-only.

stream: fix stream not wanted bug #2095

Merged
merged 2 commits into from
Feb 7, 2020
Merged

stream: fix stream not wanted bug #2095

merged 2 commits into from
Feb 7, 2020

Conversation

acud
Copy link
Member

@acud acud commented Feb 7, 2020

This PR fixes a bug where a stream that is no longer wanted would result in two WantedHashes messages sent to the upstream peer.
Certain side effect observed is that the downstream peer would still try to seal the batch in any case, and this in turn causes some other data races on the server side.

@acud acud added this to the 0.5.6 milestone Feb 7, 2020
@acud acud requested review from janos, zelig, svetomir and pradovic February 7, 2020 14:38
@acud acud self-assigned this Feb 7, 2020
@@ -481,6 +481,7 @@ func (r *Registry) clientHandleOfferedHashes(ctx context.Context, p *Peer, msg *
if err := p.Send(ctx, wantedHashesMsg); err != nil {
protocols.Break(fmt.Errorf("sending empty wanted hashes: %w", err))
Copy link
Contributor

Choose a reason for hiding this comment

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

return protocols.Break(fmt.Errorf("sending empty wanted hashes: %w", err)) ?

Copy link
Contributor

@pradovic pradovic Feb 7, 2020

Choose a reason for hiding this comment

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

Might be my bug, a leftover when I was introducing break errors 🙈

Copy link
Contributor

@pradovic pradovic left a comment

Choose a reason for hiding this comment

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

LGTM, nice one!

Copy link
Member

@janos janos left a comment

Choose a reason for hiding this comment

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

Great find. LGTM.

@acud acud merged commit 80acd99 into master Feb 7, 2020
@acud acud deleted the fix-stream-no-want-no-mo branch February 7, 2020 15:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants