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

Initial prototype for AbortablePromise #415

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

briancavalier
Copy link
Member

DO NOT MERGE: This is a prototype of an AbortablePromise, for discussion and iteration to prove the idea.

This adds a new AbortablePromise subtype that has an abort() method which can send a signal from a consumer back toward the root of the promise graph to the producer indicating that the consumer wishes to abort the task that is computing the promise's value. This can be used to stop long-running or resource-consuming tasks when a consumer decides it no longer wants the result. For example: XHR request.

Initial prototype

The initial implementation isn't ideal but is headed in the right direction. The abort functionality should be provided at the handler level, and a thin wrapper for it provided at the AbortablePromise.prototype level. The initial prototype does that, but by stashing an additional property on the existing handler. Ideally, we would have a new type of handler, eg AbortableHandler, that provided the functionality.

Open questions

This prototype doesn't provide AbortablePromise.resolve or AbortablePromise.reject. I believe that it will need to. The reason is for API consistency: APIs that return AbortablePromise may need to return resolved and/or rejected promises, and returning Promise.resolve/reject would return a promise without an abort() method, which would break callers of those APIs.

Promise.all and Promise.race may also need abortable analogs that return an AbortablePromise. They would also need to handle abortable thenables in their input arrays, aborting them when the returned promise is aborted. That may be a slippery slope into needing to detect abort in many more places (when.map, when.filter, when.reduce, etc etc etc).

cc @mjackson

@briancavalier
Copy link
Member Author

Simple test program here: https://gist.github.com/briancavalier/f606b6af9b35855c092f

var handler = this._handler;

if (typeof handler.abort !== 'function') {
return;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of returning undefined here, should we return a promise for the previous abortResult?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good catch. Either that, or maybe return Promise.resolve(this), ie a non-abortable view of the AbortablePromise instance?

@mjackson
Copy link

This looks great @briancavalier ! Thank you for taking this on.

This prototype doesn't provide AbortablePromise.resolve or AbortablePromise.reject. I believe that it will need to. The reason is for API consistency

It seems like if AbortablePromise does everything that Promise does we'll end up with a fairly complete parallel Promise implementation, just with an abort method on the prototype.

If closing over things as I did in my initial hack isn't an option, and I agree that it may not be due to performance reasons, then it may make more sense to evolve Promise instead of creating something new. Yes, it would break things. But that's why we have major version numbers ;)

@briancavalier
Copy link
Member Author

It seems like if AbortablePromise does everything that Promise does we'll end up with a fairly complete parallel Promise implementation, just with an abort method on the prototype.

Yeah, and that'd be a bummer. The real issue is all the other useful operations around promises which aren't currently abort-aware. If abort is not a first-class citizen, then literally every API that wants to take advantage of abort would either have to fail-fast when passed non-abortable promises (not really an option!), or branch based on the presence/absence of abort. That's a slippery slope, since adding the next new one-off feature would mean having to add another branch. Which is why ...

it may make more sense to evolve Promise instead of creating something new

... I'm starting to think that's actually the better option, too :) Then all promises (for now, all when.js promises) will have an abort method. For Promise, it would basically be a noop. That'd avoid lots of sniffing around for abort methods, and avoid creating a complete, parallel AbortablePromise implementation

@mjackson
Copy link

For Promise, it would basically be a noop.

For resolved promises, yes. But maybe unresolved promises should reject?

@briancavalier
Copy link
Member Author

But maybe unresolved promises should reject?

I think vanilla Promise rejecting would grant too much authority to consumers by default: any consumer could force any promise to reject, potentially affecting other (unsuspecting) consumers in the same promise graph. What's nice about AbortablePromise being a separate concept is that the producer can choose to grant that authority or not--in fact, it forces the producer to make an effort to grant it by choosing AbortablePromise over Promise.

I'd def like to have a go at another prototype where all promises have .abort(), Promise makes it a noop, and AbortablePromise implements the abort functionality. Not sure when I can get to that, tho ... might be next week, or might be sometime after Dec 28.

Please do feel free to hack on this branch if you have ideas :)

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.

2 participants