Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

Remove http client timeout code #2974

Closed
achingbrain opened this issue Apr 7, 2020 · 5 comments · Fixed by #2979
Closed

Remove http client timeout code #2974

achingbrain opened this issue Apr 7, 2020 · 5 comments · Fixed by #2979
Labels
exploration pkg:http-client Issues related only to ipfs-http-client

Comments

@achingbrain
Copy link
Member

ipfs-http-client adds a timeout option to all requests which uses an AbortController to abort the request from the client side once the timeout is reached.

This conflicts with go-ipfs' global timeout option which causes the request to be aborted from the server side once the timeout is reached.

I propose we remove the timeout from the client side and implement the server side version in js-ipfs. This is local RPC, not a public facing API (in theory) so long running requests should be ok, and if client-side timeouts are important the user can implement them by using an AbortController.

One caveat is that on the CLI go-ipfs returns a non-zero exit code when the timeout is reached, over HTTP it (probably) sends an error in the trailers for streaming responses, which we know no browser supports, so it looks like the request ends normally. When we implement the ndjson value/error wrappers discussed at the IPFS team week earlier in the year it'll throw instead of truncating the stream.

Thoughts, objections etc? @hugomrdias @alanshaw

@achingbrain achingbrain changed the title Remove client timeout code Remove http client timeout code Apr 7, 2020
@achingbrain achingbrain added the pkg:http-client Issues related only to ipfs-http-client label Apr 7, 2020
@achingbrain
Copy link
Member Author

node-fetch went through a similar process: node-fetch/node-fetch#523 though it looks like they opened an issue on whatwg/fetch around adding a timeout option that boiled do to "no, the user can use an AbortController" so at least we'd be consistent there.

@hugomrdias
Copy link
Member

sound good to me!
we should track the go cli exit code and the trailer headers issues some where
/cc @Stebalien

@alanshaw
Copy link
Member

alanshaw commented Apr 7, 2020

Timeout should definitely be sent to the server. In the few places we had timeout implemented in the client I believe we are (or were) sending it anyway.

👍 I'd love to see it implemented in js-ipfs.

@achingbrain
Copy link
Member Author

In the few places we had timeout implemented in the client I believe we are (or were) sending it anyway.

Ah, I see - I think we used to, but when ky was refactored away, the replacement wrapped every request with a timeout (if specified), so I suppose we are still sending the timeout, but the client one is likely to expire before the server side one so we'd never know.

@Stebalien
Copy link
Member

SGTM. I prefer implementing the timeout on both, just to be safe, but sending the timeout to the server gives the server some information on how long the client wishes to spend performing the request.

@achingbrain achingbrain linked a pull request Apr 14, 2020 that will close this issue
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
exploration pkg:http-client Issues related only to ipfs-http-client
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants