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

Setting a Default Request Timeout for REST API Calls #49

Merged
merged 3 commits into from
Dec 9, 2021

Conversation

onelapahead
Copy link
Contributor

Testing FireFly DX in an env with a bad network, I noticed DX would "hang" for roughly 2m10s on each req attempt until ETIMEDOUT was thrown:

2021-12-09T18:41:37.758Z [INFO ]: FireFly Data Exchange running on http://0.0.0.0:5000 (API) and https://0.0.0.0:5001 (P2P) - log level "DEBUG" app.ts
2021-12-09T18:42:06.511Z [INFO ]: New WebSocket client connected (client count: 1) app.ts
2021-12-09T18:42:06.511Z [INFO ]: New WebSocket delegate assigned app.ts
2021-12-09T18:44:36.353Z [DEBUG]: post https://firefly-dx.example.com/api/v1/messages utils.ts
2021-12-09T18:46:46.263Z [ERROR]: post https://firefly-dx.example.com/api/v1/messages attempt 0 [undefined] Error: connect ETIMEDOUT 10.1.6.180:443
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1159:16) utils.ts
2021-12-09T18:46:46.765Z [DEBUG]: post https://firefly-dx.example.com/api/v1/messages utils.ts
2021-12-09T18:48:57.334Z [ERROR]: post https://firefly-dx.example.com/api/v1/messages attempt 1 [undefined] Error: connect ETIMEDOUT 10.1.6.180:443
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1159:16) utils.ts
2021-12-09T18:48:57.836Z [DEBUG]: post https://firefly-dx.example.com/api/v1/messages utils.ts
2021-12-09T18:51:08.406Z [ERROR]: post https://firefly-dx.example.com/api/v1/messages attempt 2 [undefined] Error: connect ETIMEDOUT 10.1.6.180:443
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1159:16) utils.ts
2021-12-09T18:51:08.907Z [DEBUG]: post https://firefly-dx.example.com/api/v1/messages utils.ts
2021-12-09T18:53:19.478Z [ERROR]: post https://firefly-dx.example.com/api/v1/messages attempt 3 [undefined] Error: connect ETIMEDOUT 10.1.6.180:443
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1159:16) utils.ts
2021-12-09T18:53:19.979Z [DEBUG]: post https://firefly-dx.example.com/api/v1/messages utils.ts
2021-12-09T18:55:30.550Z [ERROR]: post https://firefly-dx.example.com/api/v1/messages attempt 4 [undefined] Error: connect ETIMEDOUT 10.1.6.180:443
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1159:16) utils.ts
2021-12-09T18:55:31.051Z [INFO ]: Event emitted message-failed/2962a5f9-d6b6-4d19-9746-2dfedc054d1e app.ts
2021-12-09T18:55:31.052Z [ERROR]: Failed to deliver message Error: connect ETIMEDOUT 10.1.6.180:443 handlers/messages.ts

According to axios/axios#1739 and https://axios-http.com/docs/req_config, setting this should make it so these reqs fail much more quickly.

Signed-off-by: hfuss <haydenfuss@gmail.com>
@nguyer
Copy link
Contributor

nguyer commented Dec 9, 2021

1 second does seem pretty aggressive. Is there a reason this particular value was chosen?

@onelapahead
Copy link
Contributor Author

onelapahead commented Dec 9, 2021

@nguyer none really short of I expect DX's themselves to be quick w/ these responses so the real latency they experience should only be due to TLS handshakes, geography / speed of light, and load balancers / proxies in-between.

I'd be alright something more like 2s - 10s if need be, just don't think it should be much longer than 10s.

Signed-off-by: hfuss <haydenfuss@gmail.com>
@gabriel-indik
Copy link
Contributor

gabriel-indik commented Dec 9, 2021

Agree with the observation and the change that followed bumping the timeout from 1 to 5 seconds. Note that these calls are used not only for sending messages, but also for transferring files. On slow connections large size files could take several seconds to make it to the other side. We don't want to risk timing out the connection while files are being transfer.

@onelapahead
Copy link
Contributor Author

@gabriel-indik ah thanks Gabriel! Makes sense. In that case then I could even see us going up to 30s.

Another option is we set this timeout differently for messages vs blobs using the request config. So messages could be 5s, but blobs 60s... thoughts?

Signed-off-by: hfuss <haydenfuss@gmail.com>
@gabriel-indik
Copy link
Contributor

That's a great idea, we could make the code adjust the timeout dynamically depending on the size of the payload to be transferred. But in principle, using 5 seconds as set in this PR I think is pretty safe for now. Unless someone wants to transfer a file that is over a hundred MB, we should be good.

@gabriel-indik gabriel-indik merged commit 69ad14c into hyperledger:main Dec 9, 2021
@onelapahead
Copy link
Contributor Author

This worked as expected:

2021-12-12T23:36:27.521Z [ERROR]: post https://firefly-dx.example.com/api/v1/messages attempt 4 [undefined] Error: timeout of 5000ms exceeded
    at createError (/firefly-dataexchange-https/node_modules/axios/lib/core/createError.js:16:15)
    at RedirectableRequest.handleRequestTimeout (/firefly-dataexchange-https/node_modules/axios/lib/adapters/http.js:328:16)
    at RedirectableRequest.emit (events.js:400:28)
    at Timeout.<anonymous> (/firefly-dataexchange-https/node_modules/follow-redirects/index.js:164:12)
    at listOnTimeout (internal/timers.js:557:17)
    at processTimers (internal/timers.js:500:7) utils.ts

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.

None yet

3 participants