interface-ipfs-core: timeout tests pass without implementation #3161
Comments
|
Thank you for submitting your first issue to this repository! A maintainer will be here shortly to triage and review.
Finally, remember to use https://discuss.ipfs.io if you just need general support. |
|
I think this is a hangover from before timeouts were implemented in js-ipfs, so the client also has a timeout.
Sounds good, we can add an option to the http client. |
achingbrain
added
kind/enhancement
|
This might be better done if we just remove the default timeout. @koivunej could you try removing the I'm trying to follow your instructions but I'm getting an error |
Do not specify a default timeout - since requests are now abortable, killing the connection (e.g. ctrl+c) will tear down long-lived resources on the node. fixes: #3161
|
@achingbrain I applied this: @@ -110,7 +110,7 @@
/** @type {ClientOptions} */
const opts = normalizeInput(options)
super({
- timeout: parseTimeout(opts.timeout) || 60000 * 20,
+ timeout: parseTimeout(opts.timeout),
headers: opts.headers,
base: normalizeInput(opts.url).toString(),
handleError: errorHandler,With this patch and how we currently run the interface-http-core tests now in rust-ipfs/conformance (we simplified from two repos to just one), I cannot see any difference in outcome before or after removing the timeout implementation from Tried also adding an "assertion" that I got a single random test failure when running When running the test in loop with: It passes every time in 300-400ms: If it passes normally in 300-400ms and the timeout is 2000ms, I would expect testing the build without any This hangs forever as expected with the timeout parameter processing commented out. |
|
Fixed repro instructions (I had used wrong name for ipfs-http of course :):
Patch to disable diff --git a/http/src/v0/block.rs b/http/src/v0/block.rs
index f2ac799..22d5819 100644
--- a/http/src/v0/block.rs
+++ b/http/src/v0/block.rs
@@ -32,10 +32,8 @@ async fn get_query<T: IpfsTypes>(
let cid: Cid = query.arg.parse().map_err(StringError::from)?;
let data = ipfs
.get_block(&cid)
- .maybe_timeout(query.timeout.map(StringSerialized::into_inner))
.await
.map_err(StringError::from)?
- .map_err(StringError::from)?
.into_vec();
let response = Response::builder().body(data);
diff --git a/http/src/v0/dag.rs b/http/src/v0/dag.rs
index c93eb78..84ad57e 100644
--- a/http/src/v0/dag.rs
+++ b/http/src/v0/dag.rs
@@ -129,9 +129,7 @@ async fn inner_resolve<T: IpfsTypes>(
let path = IpfsPath::try_from(opts.arg.as_str()).map_err(StringError::from)?;
let (current, _, remaining) = walk_path(&ipfs, path)
- .maybe_timeout(opts.timeout.map(StringSerialized::into_inner))
.await
- .map_err(StringError::from)?
.map_err(StringError::from)?;
let remaining = { |
Do not set up a client-side timeout, instead rely on the server-side version. The global timeout is still respected, users can use an AbortSignal to do timeouts on a per-call basis if desired. fixes: #3161
Do not set up a client-side timeout, instead rely on the server-side version. The global timeout is still respected, users can use an AbortSignal to do timeouts on a per-call basis if desired. Also brings refs API in line with go-IPFS - timeouts are recorded in the `.err` prop of the output instead of the whole API call throwing a `TimeoutError` fixes: #3161
Do not set up a client-side timeout, instead rely on the server-side version. The global timeout is still respected, users can use an AbortSignal to do timeouts on a per-call basis if desired. Also brings refs API in line with go-IPFS - timeouts are recorded in the `.err` prop of the output instead of the whole API call throwing a `TimeoutError` fixes: #3161
From package-lock.json:
interface-ipfs-core
And:
ipfs-http-client
Severity:
Description:
Run the tests before implementing the any
timeout=1msquery parameter handling on rust-ipfs.Steps to reproduce the error:
cargo build -p httprust.shunder ipfs-rust-conformance ashttpIPFS_RUST_EXEC="$(pwd)/rust.sh" npm test -- --grep 'should respect timeout option when'All tests should pass even if the functionality has not been implemented. There are probably issues with following the above instructions, so no harm in not trying. Good to print them out for once at least :)
I suspect that ipfs-http-client library internally handles the timeout while also rendering it to the query parameter. Perhaps the http client level timeout should be disabled when doing these timeout tests? If there is such way in the ipfs-http-client, I couldn't find it in https://github.com/ipfs/js-ipfs/blob/master/packages/ipfs-http-client/README.md#global-timeouts.
The text was updated successfully, but these errors were encountered: