Skip to content

Comments

fix(libp2phttp): bound NewStream timeout#3225

Merged
MarcoPolo merged 2 commits intolibp2p:masterfrom
algorandskiy:http-new-stream-context
Mar 19, 2025
Merged

fix(libp2phttp): bound NewStream timeout#3225
MarcoPolo merged 2 commits intolibp2p:masterfrom
algorandskiy:http-new-stream-context

Conversation

@algorandskiy
Copy link
Contributor

libp2phttp.streamRoundTripper.RoundTrip uses a context from http.Request when establishing a new stream.
Suppose a user expects to fetch very large files with multiplexed http so they set very large timeout into http.Request context. Then suppose one of the servers does not respond to protocols negotiation for some reason (for example by reaching out max connections set via resource manager).
This makes NewStream to stuck in reading server response for potentially for very long time until the context with high timeout value expires.

Tested manually.

@algorandskiy algorandskiy force-pushed the http-new-stream-context branch from 5e6a652 to 802eb9d Compare March 7, 2025 22:47
@sukunrt sukunrt requested a review from MarcoPolo March 17, 2025 16:29
@MarcoPolo
Copy link
Collaborator

Thanks.

I like this change, but I don't love that it brings in basichost as a dependency. It might be better to simply copy the default timeout to this package instead.

@algorandskiy
Copy link
Contributor Author

Thanks for the review. Fixed.

@MarcoPolo MarcoPolo merged commit a743f9f into libp2p:master Mar 19, 2025
9 checks passed
@sukunrt sukunrt mentioned this pull request Mar 24, 2025
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