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

No ability to set a Timeout behaviour #4122

Closed
nickgrealy opened this issue Jun 5, 2023 · 13 comments
Closed

No ability to set a Timeout behaviour #4122

nickgrealy opened this issue Jun 5, 2023 · 13 comments
Assignees
Labels
enhancement New feature or improvement. fixed/complete This Bug is fixed or Enhancement is complete and published. minor-bump Planned for the next minor version bump. v6 Issues regarding v6

Comments

@nickgrealy
Copy link

nickgrealy commented Jun 5, 2023

Ethers Version

6.4.1

Search Terms

cancel, abort signal, https agent

Describe the Problem

I cannot find a way to cancel a Json RPC request inflight, with the v6 library (specifically to set a timeout).

All the methods I've tried, appear not to work, and I can't see anything in the documentation.

Attempt #1 (Bug)

Attempt: Set the timeout field to 1 (ms) on the FetchRequest.
Expected: requests to cancel effectively immediately.
Result: Requests still complete successfully after a duration of approx half a second.

const fetchRequest = new ethers.FetchRequest(network)
fetchRequest.timeout = 1
fetchRequest.preflightFunc = async (req: ethers.FetchRequest): Promise<ethers.FetchRequest> => {
  req.timeout = 1
  return req
}

Attempt #2 (Bug)

Attempt: Call the FetchRequest.cancel() function.
Expected: requests to cancel mid flight
Result: request throws error "request has not been sent" (I have a long request, I'd expect it to at least have been initialised after 50ms)

const fetchRequest = new ethers.FetchRequest(network)
fetchRequest.preflightFunc = async (req: ethers.FetchRequest): Promise<ethers.FetchRequest> => {
  setTimeout(() => req.cancel(), 50)
  return req
}

Attempt 3 (Feature request)

Attempt: Supply an AbortSignal or FetchCancelSignal in the preflightFunc
Expected: ability to set a custom abort signal
Result: there is no documented or visible way to set an abort signal

Note: Additionally, #signals are not copied during the clone() function.

Attempt 4 (Feature request)

Attempt: Supply a custom Https Agent
Expected: most libraries provide the ability to override or set the underlying Https Agent (e.g. in v5)
Result: this is no longer supported

Code Snippet

Code snippets provided inline.

Contract ABI

N/A

Errors

No response

Environment

node.js (v12 or newer)

Environment (Other)

Node: v18.3.0

@nickgrealy nickgrealy added investigate Under investigation and may be a bug. v6 Issues regarding v6 labels Jun 5, 2023
@nickgrealy nickgrealy changed the title Cannot set an AbortSignal or Timeout No ability to set a Timeout behaviour Jun 5, 2023
@nickgrealy
Copy link
Author

nickgrealy commented Jun 5, 2023

Currently trying this code to implement a 1 millisecond timeout. I expect all requests to be cancelled, but I can see responses are still being received successfully, AFTER the cancel request is called.

Is this because the code is expecting the req.cancel() to propagate an error... ???

const fetchRequest = new ethers.FetchRequest(network)
const timeoutMS = 1
fetchRequest.preflightFunc = async (req: ethers.FetchRequest): Promise<ethers.FetchRequest> => {
  ;(req as any).timeoutRef = setTimeout(() => {
    try {
      req.cancel()
    } catch (error) {
      console.error(`Cancelled request after ${timeoutMS.toLocaleString()} milliseconds`)
    }
  }, timeoutMS)
  return req
}

// intercept the response
fetchRequest.processFunc = async (
  req: ethers.FetchRequest,
  resp: ethers.FetchResponse
): Promise<ethers.FetchResponse> => {
  if ((req as any).timeoutRef) {
    clearTimeout((req as any).timeoutRef)
  }
  // ... the response still comes through successfully, and is handed back to waiting promise, despite a 1 millisecond timeout
  return resp
}

@ricmoo
Copy link
Member

ricmoo commented Jun 8, 2023

The Signalling was added as a stub to add in the future. I can look at this now.

Each platform has its own support for cancelling requests, and some are actually honoured (i.e. stop the request and reclaim socket resources) while some simply discard the results when they finally arrive. It will still be up to the platform to decide how to handle this, but at the very least, Ethers should provide a standard and consistent experience.

@dogukanakkaya
Copy link

dogukanakkaya commented Jun 30, 2023

I needed the same thing with contract function calls. It's not just about cancelling requests after a specific timeout but it's also useful for cleanups in frontend. I saw some signal implementations for request cancellation but seems like they're for internal usages.

It'd be nice if we get a similar usage like fetch, axios etc. Passing an AbortController.signal property.

@ricmoo
Copy link
Member

ricmoo commented Jul 1, 2023

I had a lot of ideas I tried out for v6, but it’s hard because of the way TypeScript and Promises work together.

But I have some ideas for v7 to try out, which will be that each provider method (and Contract overrides) will accept an AbortController, as well as a customData, which can be included in all errors emitted and more importantly in the getUrlFunc so additional data can be passed into custom fetching.

Any other ideas for options to pass in?

@dogukanakkaya
Copy link

No need to pass all AbortController object. I believe passing an AbortSignal should be enough. So in the application side developers can just use it for timeout and cleanup purposes:

const abortController = new AbortController()

// which would internally listen for `signal.addEventListener('abort', () => ...)` and cancel the requests
contract.func(...args, { signal: abortController.signal })

setTimeout(() => abortController.abort(), 3000) // abort after 3 seconds

Same for unmounts in frontend, React for example:

useEffect(() => {
  const abortController = new AbortController()
  
  // which would internally listen for `signal.addEventListener('abort', () => ...)` and cancel the requests
  contract.func(...args, { signal: abortController.signal })

  return () => abortController.abort() // abort if component is unmounted
}, [])

@ricmoo
Copy link
Member

ricmoo commented Jul 1, 2023

Oh, sorry. Yes, that’s what I meant. There is an Ethers object for that, so that the interface is the same between platforms. :)

@andkom
Copy link

andkom commented Feb 19, 2024

There is a bug in utils/geturl.ts: timeout event from socket is ignored but must be handled manually.

request.setTimeout(timeout, function() {
    request.abort();
});

@bytesbay
Copy link

@andkom Hey, do you also have endless requests sometimes?

@andkom
Copy link

andkom commented Mar 17, 2024

@bytesbay yes, but not fixed with the code above

@sebastiantf
Copy link

Is this timeout feature not implemented yet on v6? Is it planned for v7

@ricmoo
Copy link
Member

ricmoo commented Apr 24, 2024

This is still planned for v6. :)

@ricmoo ricmoo added enhancement New feature or improvement. on-deck This Enhancement or Bug is currently being worked on. minor-bump Planned for the next minor version bump. next-patch Issues scheduled for the next arch release. and removed investigate Under investigation and may be a bug. labels Apr 24, 2024
@ricmoo
Copy link
Member

ricmoo commented Apr 25, 2024

I've added some logic to handle timeout and cancel in FetchRequest consistently between the browser and Node.

I'd love an extra set of eyes on it. I can also get a beta build published on npm if there is interest.

ricmoo added a commit that referenced this issue Jun 3, 2024
@ricmoo
Copy link
Member

ricmoo commented Jun 4, 2024

This has been published in v6.13.0.

Thanks! :)

@ricmoo ricmoo closed this as completed Jun 4, 2024
@ricmoo ricmoo added fixed/complete This Bug is fixed or Enhancement is complete and published. and removed on-deck This Enhancement or Bug is currently being worked on. next-patch Issues scheduled for the next arch release. labels Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement. fixed/complete This Bug is fixed or Enhancement is complete and published. minor-bump Planned for the next minor version bump. v6 Issues regarding v6
Projects
None yet
Development

No branches or pull requests

6 participants