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

Optionally breaking the circuit without having to wait for the route handler to finish #83

Open
2 tasks done
JaneJeon opened this issue Apr 7, 2022 · 6 comments
Open
2 tasks done
Labels
good first issue Good for newcomers

Comments

@JaneJeon
Copy link

JaneJeon commented Apr 7, 2022

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the feature has not already been requested

🚀 Feature Proposal

So I saw that on the caveats section, the handler will just wait for the response to be generated anyway and just send an error if the timeout has been reached.

I wonder, with something like p-timeout and p-cancelable, would it not be possible to cancel the Promise without having to wait for it?

For example, you could simply check that a route handler promise has a .cancel method (as is the case when you use p-cancelable), and cancel those promises when the timeout is hit; and when it’s a “vanilla” promise, just fall back to the default behaviour as-is.

That way, we can support “not having to wait for route handler to finish” and actually use the timeout as a timeout while not breaking anything (minor semver) and still supporting the existing behaviour.

Motivation

No response

Example

No response

@mcollina
Copy link
Member

mcollina commented Apr 7, 2022

This looks like a good feature to add! However it might be better implemented on top of AbortController. Could you take a look at that API and see how it could be implemented?

@JaneJeon
Copy link
Author

JaneJeon commented Apr 7, 2022

Ah, I've actually used ptimeout, pcancelable and abortcontroller all extensively recently lol

So basically, all you need as the "base" is a promise that can be cancelled, whether thru accepting .cancel() or AbortSignal. There are times where either is better.

For example, for working with modules with built-in abortsignal integration (e.g. native node modules, got, etc), it's basically perfect.

But basically, the API for making promises would look something like this:

const handler = async (req, res) => {
  // this can either be from (req, res, ...) or from a request-scoped storage (fastify. ...);
  // and given that we don't want users to be calling .abort() - we'd rather have them throw - we could just pass the abortsignal directly
  const signal = abortController.signal

  // For modules that support abortcontroller:
  await doPromise(input, { signal })

  // For modules that support .cancel() or any other alternate method of canceling
  const promise = doPromise2()
  signal.on('abort', () => promise.cancel())
  await promise

  // For modules that can't be cancelled in any way:
  if (signal.aborted) throw new Error('timed out!')
  await doWork() // we can just be content with being able to not run some work when aborted, rather than waiting for the promise to reach the end.
}

Just a rough idea of what it'd look like. Many ways to skin the cat, but for optimal results you'd use either modules that support abortsignal/cancellation, or implement the if (signal.aborted) check within your handler to "gate" it.

@stale
Copy link

stale bot commented Apr 27, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 27, 2022
@JaneJeon
Copy link
Author

:/

@stale stale bot removed the stale label Apr 28, 2022
@jsumners
Copy link
Member

Would you like to send a Pull Request to address this issue? Remember to add unit tests.

@JaneJeon
Copy link
Author

Currently have a lot a lot on my plate - work and personal - so if I do get around this, it'll be probably months before I can even block some time for this, but I believe it should still be kept open and not closed by stalebot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants