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

Fix so that HTTP request don't wait for responses indefinitely #506

Merged

Conversation

torao
Copy link
Contributor

@torao torao commented Nov 22, 2022

Description

Currently, there are a few codes that use the golang http.Client with no timeout in Ostracon. This can cause stability and safety problems for nodes, as they behave as that they are waiting indefinitely when the destination doesn't respond.

This PR replaces HTTP requests naively used without timeouts with those with timeouts. However, this is only for code related to http.Client{} or related http.Get(), and doesn't consider other TCP or Unix communications.

This fix sets the timeout as follows:

  • If it has a general idea of what timeout to expect, apply it.
  • Apply 30 seconds for requests used in UPnP. It's sufficient for a human to actually enter the command and ensure that UPnP is not working.
  • Apply 60 seconds for requests used in testing. This is because the load in a CI environment can cause even correct behavior to take a significant amount of time to respond.
  • Do nothing for use cases where callers are using with and without timeouts, i.e., explicitly expecting no timeout.

The only places affected other than the test code are the CLI commands UPnP prove→discover and dump.

@torao torao self-assigned this Nov 22, 2022
@torao torao added the C: enhancement Classification: New feature or its request, or improvement in maintainability of code label Nov 22, 2022
@torao torao marked this pull request as draft November 22, 2022 12:26
@torao torao marked this pull request as ready for review November 22, 2022 12:35
* If it have a general idea of what timeout to expect, apply it.
* Applied 30 seconds for requests used in UPNP.
* Applied 60 seconds for requests used in testing.
* Do nothing for use cases where callers are using with and without timeouts.
@torao torao force-pushed the feature/add_timeout_to_default_http_request branch from 5ad23b1 to 72d4ebe Compare November 24, 2022 02:42
@codecov
Copy link

codecov bot commented Nov 24, 2022

Codecov Report

Merging #506 (f690d36) into main (e6f57a1) will increase coverage by 0.04%.
The diff coverage is 91.66%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #506      +/-   ##
==========================================
+ Coverage   65.39%   65.44%   +0.04%     
==========================================
  Files         278      279       +1     
  Lines       37995    37984      -11     
==========================================
+ Hits        24846    24857      +11     
+ Misses      11328    11314      -14     
+ Partials     1821     1813       -8     
Impacted Files Coverage Δ
rpc/jsonrpc/client/test_util.go 0.00% <0.00%> (ø)
libs/net/http.go 100.00% <100.00%> (ø)
crypto/sr25519/pubkey.go 35.89% <0.00%> (-7.70%) ⬇️
blockchain/v2/routine.go 63.91% <0.00%> (-5.16%) ⬇️
abci/client/socket_client.go 72.09% <0.00%> (-0.88%) ⬇️
blockchain/v2/reactor.go 34.71% <0.00%> (-0.30%) ⬇️
p2p/conn/connection.go 79.80% <0.00%> (-0.20%) ⬇️
proxy/multi_app_conn.go 47.66% <0.00%> (ø)
crypto/vrf/vrf_libsodium.go
crypto/vrf/vrf_r2ishiguro.go 100.00% <0.00%> (ø)
... and 9 more

tnasu
tnasu previously approved these changes Nov 24, 2022
p2p/switch_test.go Outdated Show resolved Hide resolved
p2p/upnp/upnp.go Outdated Show resolved Hide resolved
@ulbqb ulbqb self-requested a review November 25, 2022 03:19
@torao torao merged commit f92e500 into Finschia:main Nov 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: enhancement Classification: New feature or its request, or improvement in maintainability of code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants