tc39 / proposal-cancelable-promises

Former home of the now-withdrawn cancelable promises proposal for JavaScript
Other
375 stars 29 forks source link

CancelToken.none? #56

Closed domenic closed 7 years ago

domenic commented 8 years ago

I was going to write up a spec for a never-canceled CancelToken.none, which could be used as a default argument for cancel token-accepting functions that want to do nothing. However, I realized I couldn't find any realistic use cases. Since Promise.withCancelToken and await.cancelToken both handle undefined well, and web platform APIs will always make the cancel token option, it seems likely that most functions that accept cancel tokens will work just fine with undefined passed instead. It seems very rare that you'll actually want to use CancelToken.none.

Can anyone see cases where this logic fails?

benjamingr commented 8 years ago

Well in C# CancelToken.none is useful because it has the same API as regular cancellation tokens so the callee site doesn't need to care if it's passed a real token or not.

If I pass undefined I have to do something like

if(token !== undefined) {
   token.promise.then(() => freeResourcesFromThis());
}

Instead of just

   token.promise.then(() => freeResourcesFromThis());
bergus commented 8 years ago

This assumes that Promise.withCancelToken and await.cancelToken are the primary mechanisms to register cancellation handlers - and they should indeed be, as .promise.then is prone to leaks. I agree with @domenic here, we'll hardly need this.

benjamingr commented 8 years ago

I think .promise.then will be pretty frequently used to register handlers. I don't think that my use case is enough to motivate a whole new fixed global (just like we don't have Function.noop) but I definitely think it'll be used if it'll be there.

domenic commented 8 years ago

@benjamingr I understand how it can be used in the abstract. Can you give an example of such registration, which does not benefit from instead using the cancel token in other ways?

benjamingr commented 8 years ago

Well, pretty much anywhere .Register is used in .NET land. Typically you already have an async API and you want to add cancellation in order to prevent some side effect rather than distrupt the function's flow.

Let's say we have an action that writes to a journal, we want to write to the journal in bulk but when cancellation happens we want to switch to writing items one by one in an unbuffered way. When the process has to stop it still has time to perform cleanup - in any case it should not drop writes until it has to.

async function updateQueue(producer, token) {
  const journal = new BufferedJournal();
  const regularJournal = new UnbufferedJournal();
  let write = (msg) => journal.write(message);
  // when we cancel, flush all writes and convert writes to regular writes that are unbuffered
  token.promise.then(() => { 
    journal.flush(); 
    write = msg => regularJournal.write(message);
    producer.signalDontBringMoreData();
   });
  for await (let message of producer.getRealTimeUpdateMessages()) {
    await write(message);
  }
}

Note that if cancellation occurs we do not want to stop writing all the messages we got - the producer might be doing its own caching and we don't want to lose data - on cancellation we simply want to fall back to unbuffered mode and then complete the function normally.

This is just one example (I have several) but it is loosely based on very real code we have in several places in production. A lot of our systems are microservices that have to go down several times a week for OS updates - when the service goes down it gets 5 minutes to do cleanup - so we ask the producer to stop sending things, finish writing in non-bulk mode and terminate gracefully. We do have code to deal with things like "computer was shut down" but it is a lot more expensive and using .Register really helps.

I'll send you some real C# code in an email reply to this thread since I'm probably not in discretion to just post it whole here. It uses this pattern but it's also thread safe.

bergus commented 8 years ago

This seems to be just another example where you leak the cancellation handler (and with it the journals) in case the producer stops without the token being cancelled. This better would be written without .promise.then as

async function updateQueue(producer, token) {
  let journal = new BufferedJournal();
  const regularJournal = new UnbufferedJournal();
  for await (let message of producer.getRealTimeUpdateMessages(token)) {
//         instead of `.signalDontBringMoreData()`, pass token ^^^^^
    if (journal && token.requested) {
      journal.flush();
      journal = null;
    }
    await (journal || regularJournal).write(message);
  }
}

or

  …
    if (token.requested) journal.flush(); // no-op if already happened
    await (token.requested ? regularJournal : journal).write(message);
  …

But you've got a point there, I don't want to test token && token.requested, so an empty token might make sense. Maybe await.cancelToken should return one instead of undefined as well?

benjamingr commented 8 years ago

I'm sorry I should have been clearer about the point of writing that code with .promise (.Register in real life).

In your code if producer.getRealTimeUpdateMessages() does not yield a message for a while you'll never enter another iteration of the loop and never flush. The messages would be lost and the more expensive recovery procedure (that takes 2 hours of CPU time) will be run. It would literally cost us hundreds of dollars a month more to use that code :)

Passing the journal to producer makes sense (and indeed it is passed to it in the constructor). In your code aborting an iterator typically does not keep running the iteration (calling .return on the iterator).

The only way this might work would be if when the producer is aborted it pushes a "poison pill" value to the iteration which is recognized and ignored (instead of a real message).

I have plenty of other examples of where I use .Register in C#, I just provided the one I had open on my screen when I saw the email about this issue. I definitely see people using .promise in cases where cancellation means keeping the function running for a while longer and performing async actions while terminating it gracefully. I can find more examples if you're interested - and this particular pattern (in "journal" here) is actually used in several other places where doing fool-proof recovery is usually a lot more expensive than letting the worker finish and switch algorithms.

I use .promise (.Register) in order to do combinators like .race too.


Anyway - that's interesting discussion but not really the topic. I'm not sure I'm in favor of CancelToken.none - we don't have a null-object object for other objects either. We don't have a Function.noop we pass to callback APIs, or an Object.empty for a sealed empty object. In C#, a CancelToken is a value type - you can't pass null instead of it so they have to provide a CancellationToken.None.

domenic commented 8 years ago

In C#, a CancelToken is a value type - you can't pass null instead of it so they have to provide a CancellationToken.None.

Ah, that's pretty valuable information. Hmm.