Unexpected try/catch/finally semantics

[chrisregnier]

After my first run at creating a couple of CAF runnables and playing with cancellation I've realized adding a finally changes the semantics in an unexpected way (to be fair this is documented but I didn't quite realize the implications until I ran into it).
Example running the following should log 'try'

let cancelToken = new CAF.cancelToken();
CAF(*main(signal) {
  try {
    console.log('try');
    yield new Promise(() => {});   // yield forever
  }
  catch (err) {
    console.log('catch');
  }
  console.log('after');
})( cancelToken.signal);
wait(5000).then(() => cancelToken.abort('hello abort'))

Meanwhile add the finally block and now this will print 'try', 'finally' (Note it does not print 'after')

let cancelToken = new CAF.cancelToken();
CAF(*main(signal) {
  try {
    console.log('try');
    yield new Promise(() => {});   // yield forever
  }
  catch (err) {
    console.log('catch');
  }
  finally {
    console.log('finally');
  }
  console.log('after');
})( cancelToken.signal);
wait(5000).then(() => cancelToken.abort('hello abort'))

This was not quite what I was expecting, and now finally blocks get triggered from 3 paths instead of the normal 2 (3rd path is the hidden cancel signal). I was actually expecting the abort to throw a special Cancel Error or even just the value passed into .abort('hello abort') and then be able to catch it and check for the special cancel error or a cancel Symbol. Obviously if you don't have a try/catch around your yields then you're not going to catch any errors and the whole runner is finished.

I can understand the behaviour makes sense since you're not really wrapping the signal promise in the try catch, but it seems weird that it acts like a hidden error and triggers the finally only. So I'm wondering if there would be value in adding a new abortWithThrow(reason) that calls the generator's throw with some kind of cancel error/symbol as part of aborting the AbortSignal.
Then this would print 'try', 'cancelled: hello abort', 'after'

let cancelToken = new CAF.cancelToken();
CAF(*main(signal) {
  try {
    console.log('try');
    yield new Promise(() => {});   // yield forever
  }
  catch (err) {
    if (err instanceof CAF.CancelError) {
      console.log('cancelled: ' + err.message)
    }
    else {
      console.log('catch');
    }
  }
  console.log('after');
})( cancelToken.signal);
wait(5000).then(() => cancelToken.abort('hello abort'))

[getify]

One thing to note: I can't do anything about how finally works... which is to say, finally always runs after a try and/or catch has run... the only way I could stop a finally that you specify inside your code from running would be to not resume the generator at all.

Moreover, the point of finally is to provide cleanup, and you pretty much always want clean up when you're stopping the run of some code, whether that's by error or by cancelation.

I'll come back to consider separately your other question of if the error thrown into the canceled function should be detectable in a special way. But just wanted to point out that your concerns around finally are... in effect... against the language feature's design, fundamentally. The conclusion would be, don't use finally if you don't want it to always run.

Your final code snippet there seems, to me, an unusual attempt to work-around not liking how finally works, by creating a non-idiomatic "after" block where you only get to that code by seeing that a special cancel-error type was sent in, and instead of actually cancelling, you just let the function proceed under some special state. It works, but I would advise against this. It's likely going to confuse others. You'll probably get "why didn't you just use finally?" questions. I certainly would if I were reviewing code with that pattern.

Perhaps it's a good idea to take a step back and contemplate why you're actually concerned about how finally works? It's a well-known feature/idiom, and it's designed for the case you're describing, so it's reasonable and responsible to really try to either conform to it, or have a really good reason to diverge.

[chrisregnier]

Sorry I gave you the wrong impression, I've been coding for well over 20 years, I'm definitely NOT trying to work around finally blocks, or to avoid the use of them :)
I'm saying that the way things are now there's effectively this code running

*main(signal) {
  signal.pr.catch(abortErr => {
    // this is kinda like code path 3 below, but can't control the flow of the throw there
  })

  try {
    ...  // code path 1. 
    // expect to run finally block next and then after block (if no errors)
    // if an error is thrown then it should go into the catch(err) block
  }
  catch (err) {
    ...  // code path 2. 
    // expect to run finally block next no matter what
    // if there's no errors thrown then after block should be run after finally block
    // if there's errors thrown then the errors bubble out and the after block is not run
  }
  catch (abortReason) {
    // code path 3, this is an imaginary block but is effectively triggered when abort is called
    // I have no way of adding code here using the try block semantics nor can I catch the abortReason and potentially changing the flow of logic from the next statement
    throw abortReason  // throw here causes it to use the finally but not fall through to the after block
  }
  finally { 
    ...  // this should always get called no matter what 
  }
  // after block
}

My problem is that code path 3 is completely hidden and yet still triggering the finally block. The only way to catch the abortReason is to add signal.pr.catch((abortReason) => ... ) but then you've got a weird mix of promises with try/catch code and all the paths eventually hit the finally.
By changing where the abort rejects within the promise chain it should be possible to inject the abortReason into the normal flow of the runner code so that it just throws like a normal error. That allows the user to catch the abort error/symbol like a normal error and handle everything gracefully within the catch and then the finally block. I basically want to catch the abortReason in the catch block, and fall through to the finally and after blocks.

I don't want to change the current behaviour though, so that's why I'm suggesting a secondary abortWithThrow that will inject the abortReason using the normal generator.throw(abortReason) (https://github.com/getify/CAF/blob/master/src/caf.src.js#L235) so it just gets passed into the catch block.

[getify]

For reference, the line of code that resumes the generator upon cancellation, so that any waiting finally can run:

CAF/src/caf.src.js

Line 55 in 3f33c5c

var ret = it.return();

The return(..) method can send in a value, but the value it sends in is (unfortunately) not accessible inside the code. The value passed in is what by default comes back out from the .return(..) call in the final iterator result, unless a finally block returns a different value.

[getify]

But to a larger point... I'm still confused by the semantic that you want the CAF-wrapped function to be resumable after it's been canceled? That doesn't really make any sense to me.

By throwing in an exception, rather than completing the generator with return(..), we are offering the opportunity for the generator to catch the thrown-in exception, swallow (or handle) it, and then it just resumes its normal flow. That doesn't at all sound like a canceled function, which is the whole point of CAF.

Allowing a finally clause to run in the cancellation case is, as you no doubt know, a standard idiomatic way of allowing clean-up. But it's not a way to trick the generator into resuming normal operations.

---

Let me ask it this way:

What logic do you want to do differently in the case of cancellation? Are you basically trying to make CAF-wrapped functions which can resume even if they've been canceled? Or are you trying to do different cleanup logic in the case of cancellation vs normal completion?

[chrisregnier]

Here's a pseudo example of the general pattern I'm after:

class ActorPool {

  _nextId = 1;
  _actors = new Map();

  constructor(actorMain) {
    this._runner = CAF(actorMain);
  }  

  spawn(...params) {
    let actor = {
      id: this._nextId++,
      cancelToken: new CAF.cancelToken(),
      params: params
    };
    actor.completion = this._runner(actor.cancelToken.signal, id, ...params);

    this._actors.set(actor.id, actor);
    return actor.id;    
  }  

  restart(id) {
    let actor = this._actors.get(id);
    actor.cancelToken.abort('restart');
    return actor.completion.catch(err => {
      if (err !== 'restart') {
        throw err;  // wasn't caused by us so pass it on
      }
      actor.cancelToken = new CAF.cancelToken();
      actor.completion = this._runner(actor.cancelToken.signal, actor.id, ...actor.params);
    }
  }
  
  cancel(id) {
    let actor = this._actors.get(id);
    actor.cancelToken.abort('cancelled');
    return actor.completion.catch(err => {
      if (err !== 'cancelled') {
        throw err;  // wasn't caused by us so pass it on
      }
      this._actors.delete(id);
    }
  }
}

let pool = new ActorPool(
  function *myActor(signal, id, ...params) {
    singal.pr.catch(abortReason => {
      // handle external events
      // these typically should do similar things as the catch cleanups, 
      //  but now I've got the logic split between promise code and the catch/finally blocks
      //  also there's no way to exit the actor gracefully
    })

    let result;
    try {
      while (true) {
        // actor logic
        result = ...
      }
    }
    catch (err) {
      // error with actor
      // example error: got expected 5xx from server, set result and handle gracefully and finish
      // example error: got unexpected 5xx from server, rethrow error
    }
    finally {
      // if happy path cleanup expensive external state
      // if error cleanup already done in catch
      // if cancelled cleanup expensive external state
      // if restart leave expensive external state
      //  but can't tell if we got in here from a cancellation, or what the cancelled reason was
    }

    // return happy path result, or gracefully catch result
    return result;
  }
)

let actorId = pool.spawn();
wait(5000);
pool.restart(actorId);
wait(5000);
pool.cancel(actorId);

Ideally I would've liked this where I have more control to do all the cleanup in the catch because you can have full contextual information on why things were aborted or errored all in the same place:

let pool = new ActorPool(
  function *myActor(signal, id, ...params) {
    // no need to listen to abort reasons this way and split up logic 
    //singal.pr.catch(abortReason => {})

    let result;
    try {
      while (true) {
        // actor logic
        result = ...
      }
    }
    catch (err) {
      if (err instanceof CAF.CancelError && err.reason === 'restart') {
        // leave expensive external state
        // gracefully exit with no rethrow 
        result = ...
      }
      else if (err instanceof CAF.CancelError && err.reason === 'cancelled') {
        // cleanup expensive external state
        rethrow err;
      }
      else {
        // normal errors. handle appropriately 
      }
    }
    finally {
      // any other final cleanup that's needed
    }

    // return happy path result, or gracefully catch result
    return result;
  }
)

So ya I'm actually trying to do both resumable cancelled functions and abort specific cleanup logic. I can see a good argument against since as you said it means the actor here could capture the abortReason and not actually abort at all, but I wonder if that's ok because it's an opt-in to the new mechanics because it uses abortWithThrow instead giving the user more power through the thrown CancelError?

[chrisregnier]

Maybe the reason I'm finding this different usecase is specially because I'm treating it as an Actor pattern and trying to have it cleanup itself instead of having an external entity try to manage state. In which case maybe the term and meaning of 'abortWithThrow' is the problem since the actor could decide not to abort. So what about calling it cancelToken.requestCancel(reason), which will throw a CancelRequest error leaving it up to the actor to decide how to handle things? If the actor rethrows the error then it calls the actual abortController.abort and ends the generator like the current behaviour, otherwise it continues on and the completion promise wouldn't get resolved at that point.

[getify]

OK, that context helps, thanks.

I still am stuck on the principle that finally is for cleanup, that's its raison d'etre. So the idea that you'd like all your cleanup in catch, or that you would split some cleanup into each of the catch and finally feels strange to me. I also feel that the idea of creating a pattern in CAF where the thing isn't actually required to cancel feels... out of place. Like, that feels like a different tool, one that creates conditionally resumable generators... like a state machine runner, etc.

But if I granted that being able to determine the "reason" for reaching finally (either cancellation or some other normal completion) would let you do different kinds of cleanup, and if we could come up with a way to make that detectable, would that be sufficient? IOW, could you move your cleanup to the finally clause, where semantically I believe it belongs?

I recognize that you may want some information (like the err) from the catch clause, and that it'd be more awkward to have to store that temporarily in an outer variable to then re-access in the finally. That is an unfortunate limitation of the language that I don't think I could do anything about. But I'm just wondering if that's a compromise worth making, here.

What I'm envisioning is something like this:

CAF(function *main(..){
   try {
      // ..
   }
   catch (err) {
      // ..
   }
   finally {
      if (CAF.isCancelled()) {
         let reason = CAF.cancellationReason();
         // ..special cancellation cleanup logic
      }
      // ..
   }
});

The CAF.isCancelled() and CAF.cancellationReason() methods (or whatever they're called) could be specially wired to only exist (or run properly) inside the finally clause while a canceled-function is cleaning itself up. Accessing them in any other time-frame (including via a time-deferred closure) would thus throw.

If we did something like that, would it be sufficient?

This is a rather narrow use-case for the generality of CAF to tackle, so I'm trying to thread the needle in terms of doing so in the most conceptually pure and limited fashion possible.

[getify]

An alternate approach without the magic methods could be to add an onFinally(..) handler as a second argument to CAF(..):

CAF(
   function *main() {
      try {
         // ..
         return "OK";
      }
      catch (err) {
         // `return` here stuffs the `err` into the `retVal` parameter of the `onFinally` handler (if any)
         return err;
      }
      finally {
         // this will always still run, first
         return 42;
      }
   },
   function onFinally(reason,retVal){
      if (reason) {
         // handle cancellation cleanup
         retVal;   // 42
      }
      else if (retVal instanceof Error) {
         // handle exception/rejection cleanup
         reason;   // undefined
      }
      else {
         // handle normal completion cleanup
         retVal;   // "OK"
         reason;    // undefined
      }
   }
);

[chrisregnier]

I think your view on finally is rather strict here. I see finally blocks as a safety hatch to capture and cleanup locally scoped resources like files/locks/connections/etc. It allows coupling a function's scope with the lifetime of some resource. This doesn't mean that all cleanup in a function belongs there, especially since there's no 'input' to the finally block so you have to maintain special state to track the path that lead there (which is partially my problem here cause the path that triggers the cancel is hidden). In many cases cleaning up where you have the contextual information is just easier.
Maybe I shouldn't have used the word 'cleanup' originally cause it was more specific to me cleaning up external state in my system, not necessarily for local resources, so I apologize if that's been overly confusing.

Either way, I've played with a couple of dead simple solutions for a couple hours now (good test cases take forever!) and I definitely agree that injecting the it.throw(CancelRequest(reason)) opens up a couple of edge cases that need more thorough work so that it doesn't reject the actual signal right away when the request is ignored, and that work doesn't look like it'll be as easy as I was hoping based on how it's currently structured. So no worries, I'll keep listening for the event from the signal.pr.catch instead. Plus if you don't think this is a useful scenario for what you want to maintain then there's no reason to force it. Although I do think your other suggestion for the CAF.isCancelled check could be useful if this topic comes up again.

Thanks anyways, have a good one.

[getify]

From this twitter thread, the idea arose to check signal.aborted to know (in the finally) if you got there via cancellation or not:

CAF(function *main(..){
   try {
      // ..
   }
   catch (err) {
      // ..
   }
   finally {
      if (signal.aborted) {
         // ..special cancellation cleanup logic
      }
      // ..
   }
});

Hadn't occurred to me that signal is in scope so we can use it instead of magical methods. That doesn't give you the cancellation reason, but that could easily be extended to be available as a property/getter/method on the signal object.

I'm sorry you felt I was being too narrow in the exploration. My default is to try to find the minimal/narrow solution to any problem, rather than throwing a large solution at a small problem.

In any case, thanks for filing the issue and for the discussion. I certainly will consider in future evolution of CAF.

[chrisregnier]

Hmm that's interesting, and rather simple enough to check. I'll give that a try.

And I wouldn't characterize your solutions/suggestions as being too narrow. You've given a number of other solutions, so much appreciated, thx!