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

protocol/ping: close ping stream when we exit the loop #1853

Merged
merged 1 commit into from
Nov 2, 2022

Conversation

MarcoPolo
Copy link
Collaborator

Changes the ping protocol to close the stream instead of reset the stream when we exit the read/write loop (happens if the peer closes their stream).

This is needed to better interop with the JS client ping implementation. That implementation will write the 32 bytes, then close the write stream, then wait for the response to be read. This is racey since the Go side will reset the stream, so the echoed bytes may have been dropped and will never be received by the JS side. Instead the JS side will see a stream reset and the ping will fail.

Came up after I made js-libp2p-webtransport close its stream after calling sink: libp2p/js-libp2p-webtransport#23 (comment) Since I'm using ping as a test this behavior made the ping test flaky.

Afaik there's no way other way to make sure data has been written to the wire besides closing the stream.

Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

Should we still reset when an error occurred?

@MarcoPolo
Copy link
Collaborator Author

The error would occur on read or write. When on read, this is the problem this is trying to fix (To be able to flush the bytes from the last write). On write, I guess we could reset, but is there any benefit?

@marten-seemann
Copy link
Contributor

Should be fine. I was assuming that we handled io.EOF differently, but apparently we don't do that.

@MarcoPolo MarcoPolo merged commit 14a3734 into master Nov 2, 2022
@MarcoPolo MarcoPolo deleted the marco/close-ping-stream branch November 2, 2022 22:55
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.

2 participants