Memory leak for long-lived tokens about cancellationtoken HOT 25 CLOSED

I have a suggestion for the Disposable idea.
In my own code, I tend to directly return the unsubscribe function from subscribe calls:

const unsub = event.subscribe(() => alert('it happened')

await something();
unsub();

That way you can define the subscribe inline while keeping the reference to the unsubsribe function.

from cancellationtoken.

While I am not a huge fan of racePromise, I think it is good enough. I would also strongly argue against overloads, this would be too messy.

We can leave canBeCancelled in, it won't hurt. I just like the API to be as slim as possible. :D

The more I think about, the more I favor the shorter method names everywhere. Technically speaking, isCancelled etc might be incorrect, but adding the "requested" everywhere does not really convey more information IMHO. Also, when creating the token, the method is called cancel, not requestCancellation. I personally would just go for the shorter names.

from cancellationtoken.

Thank you for your help and suggestions!

Version 2.0 is out now.

from cancellationtoken.

I think a good way to fix this that would be super-easy to apply in consumers would be for a method like this: whenCancelled(nevermind: Promise): Promise. It would return a new promise each time it is invoked, and that promise is put into an array kept by the CancellationToken. When canceled, this token will iterate through the array and resolve every promise in the array. But if the nevermind promise is resolved, then the promise originally returned from the new whenCanceled method will be removed from the array, thus releasing the memory allocated for the continuation that should no longer be raised.

from cancellationtoken.

BTW, I'm willing to offer a PR for this if you'll consider it.

from cancellationtoken.

Thank you for discovering and bringing that up. Glad you find the library useful. I would be delighted if you could come up with a PR, including an appropriate test. (:

from cancellationtoken.

I've found a related leak on your CONTINUE token, which by definition is a long-lived promise that never resolves. I have added tests for all the leaks I've found so far.

The fix may require some refactoring of your code. Are you ok if my PR changes your shipping code substantially, provided all your existing tests pass?
Also, I assume you're taken with the idea of avoiding semicolons. I'm trying to respect that in all my changes, but using semicolons is a hard habit to avoid. 😬

from cancellationtoken.

Hmmm... I'm beginning to see the wisdom in @dbaeumer's choice in his CancellationToken implementation to use events instead of promises. Events are well-established as a interest-on, interest-off mechanism and thereby can avoid memory leaks.

In contrast, promises simply aren't designed for never resolving/rejecting, or losing interest before a promise completes. I've spent a few hours tonight trying to write my own promise class that avoids the leak but I'm learning that implementing promises is no easy feat--and I'm bound to get some of the subtle semantics wrong enough to cause someone grief.
So while we might be able to do some sleight of hand with the original Promise class as I described where we simply abandon them, I'm now leaning toward the Event pattern instead.

@dbaeumer's event pattern is not using EventEmitter, I should mention. Instead, the event hook method returns a disposable which makes unhooking it later super convenient.

So to reiterate my prior question with a more precise plan: how do you feel about a substantial refactoring of this class to stop exposing promises altogether and use an event system instead? We might 'tack on' a promise API at the end that uses the event system underneath if we had to, but I suspect we won't. The only advantage a promise has over an event IMO is that it's awaitable, and awaiting a cancellation token doesn't seem like a common use case.

Thoughts?

from cancellationtoken.

I am all about interfaces. I'd like to think about use-cases and - as a result - the interface/API first, and then come up with a fitting implementation, crossing fingers that I can find one. So I don't mind a big refactoring and even changing the interface at all, as long as we think about the use cases first.

I am a bit opposed towards the Disposable idea. It feels natural for C# developers, because disposables are first-class citizens in .NET, but not so much for JS devs. I would probably either go with the good ol' addListener / removeListener (which would require the listener to be bound to a variable), or take the Disposable.dispose idea, but call it Registration.unlisten (or something in that direction).

It has been a while since I developed the library, but I think my reasoning behind whenCancelled was roughly the following:

const resultPromise = asyncOperation()
const fallbackResultPromise = cancellationToken.whenCancelled.then(() => fallbackResult)
const result = await Promise.race(resultPromise, fallbackResultPromise)

However, I am under the impression that we can find other ways for supporting that scenario which circumvent the memory leaks and even might reduce the boilerplate.

So far the use-cases I can come up with are the following:

• Racing a promise against a cancellation token, ideally providing some sort of having fallback values instead of throw to avoid try-catch boilerplate in certain scenarios where that is possible.
• Subscribing to the cancellation of a token. Also: unsubscribing again.
• Giving a reason as to why the cancellation happened. (any-typed)
• Having all the handy factory methods for creating cancellation tokens.

PS: Don't worry about the formatting. I will bring the repo up-to-date with a .prettierrc when I find the time. (-;

from cancellationtoken.

The wisdom for this has to go to @jrieken from whom I copied the code (https://github.com/Microsoft/vscode/blob/master/src/vs/base/common/cancellation.ts#L8)

from cancellationtoken.

How about this for the API then?

/**
 * A token that can be passed around to inform consumers of the token that a
 * certain operation has been cancelled.
 */
class CancellationToken {
  /**
   * A cancellation token that is already cancelled.
   */
  public static readonly CANCELLED: CancellationToken;

  /**
   * A cancellation token that is never cancelled.
   */
  public static readonly CONTINUE: CancellationToken;

  /**
   * Gets a value indicating whether the token has been cancelled.
   */
  public get isCancellationRequested(): boolean {
    throw new Error("not yet implemented.");
  }

  /**
   * Gets the reason why this token has been cancelled.
   */
  public get reason(): any {
    throw new Error("not yet implemented.");
  }

  /**
   * Returns a promise that is resolved when this token is canceled.
   */
  public get whenCancelled(): Promise<void> {
    throw new Error("not yet implemented.");
  }

  /**
   * Whether the token can be cancelled.
   */
  public readonly canBeCancelled: boolean;

  /**
   * Creates a {CancellationToken} and a method that can cancel it.
   */
  public static create(): { token: CancellationToken, cancel: (reason?: any) => void } {
    throw new Error("not yet implemented.");
  }

  /**
   * Create a {CancellationToken} that is cancelled when all of the given tokens are cancelled.
   *
   * This is like {Promise<T>.all} for {CancellationToken}s.
   *
   * @param tokens The tokens that the new token depends on.
   * @returns a token that depends on the given tokens.
   */
  public static all(...tokens: CancellationToken[]): CancellationToken {
    throw new Error("not yet implemented.");
  }

  /**
   * Create a {CancellationToken} that is cancelled when at least one of the given tokens is cancelled.
   *
   * This is like {Promise<T>.race} for {CancellationToken}s.
   *
   * @param tokens The tokens that the new token depends on.
   * @returns a token that depends on the given tokens.
   */
  public static race(...tokens: CancellationToken[]): CancellationToken {
    throw new Error("not yet implemented.");
  }

  /**
   * Throw a {CancellationToken.CancellationError} instance if this token is cancelled.
   */
  public throwIfCancelled(): void {
    throw new Error("not yet implemented.");
  }

  /**
   * Requests a callback when cancellation occurs.
   * If this token is already cancelled, the callback is invoked before returning.
   * @param cb The method to invoke when cancellation occurs, with the reason.
   * @returns A method that revokes the request for notification.
   */
  public register(cb: (reason?: any) => void): () => void {
    throw new Error("not yet implemented.");
  }
}

module CancellationToken {
  /**
   * The error that is thrown when a {CancellationToken} has been cancelled and a
   * consumer of the token calls {CancellationToken.throwIfCancelled} on it.
   */
  export class CancellationError extends Error {

    public constructor(

      /**
       * The reason why the token was cancelled.
       */
      public readonly reason: any
    ) {
      super(`Operation cancelled (${JSON.stringify(reason)})`) /* istanbul ignore next: see https://github.com/gotwarlost/istanbul/issues/690 */
      Object.setPrototypeOf(this, CancellationError.prototype)
    }
  }
}

export default CancellationToken

from cancellationtoken.

Well, as we already found out that whenCancelled is bound to bring troubles, we probably want to remove that thing altogether.

However, we still need some means of racing a promise against a cancellation token. I'l try to come up with something, but feel free to suggest!

Also:
isCancellationRequested --> isCancelled (more concise, same information)
register --> onCancelled (more meaningful)

For the factories I'd also like a CancellationToken.timeout(ms), but that should be trivial.

from cancellationtoken.

Well, as we already found out that whenCancelled is bound to bring troubles, we probably want to remove that thing altogether.

The promise API would only be useful for folks who never want to unsubscribe. In that case there's no memory leak because it's by design. For the non-cancelable tokens, the promise would be faked out so that it never hooks continuations. So I think we can do it -- but I'm find if we don't as well.

we still need some means of racing a promise against a cancellation token.

Why? FWIW we rarely do this in .NET, where we've had Tasks and tokens for many years. The reason being is the original request incurred some CPU and/or I/O cost, and cancelling that request takes some time to respond to in order to actually cancel the work. In the meantime, the original requestor would be polite to wait for that cancellation request to be processed rather than go on their merry way immediately, potentially incurring even more work, repeatedly till the system gets quite bogged down. So the preferred pattern has been you request cancellation, then continue awaiting the original task (without a "race with token") until the task itself reflects that cancellation has completed.

isCancellationRequested --> isCancelled (more concise, same information)

Sort of going along with my prior point, a token merely requests cancellation. It is a miscommunication to signify that some process is actually canceled. So I think the longer version is less misleading. That said, I'm willing to go along with the shorter name if you still prefer it. FWIW, in .NET we use the longer, more precise name.

register --> onCancelled

I was split on this. I'll rename.

CancellationToken.timeout(ms),

Sounds good.

from cancellationtoken.

Why? FWIW we rarely do this in .NET, where we've had Tasks and tokens for many years.

I think that in .NET this is not needed, as most (if not all) async operations have an overload where you can pass in a cancellation token. However, this is not true in JavaScript, because Promises don't support cancellation at all. However, I'd still want to do something like this:

const resultPromise = asyncOperationFromThirdPartyLib()
const result = await cancellationToken.raceAgainst(resultPromise)

It is not possible to abort asyncOperationFromThirdPartyLib, but by cancellation I want to stop caring for the result and abort early. I know I can't stop the 3rd party lib from doing it's thing once I asked for it. Maybe there is some other good way of doing that, but it is a use-case I would want to support somehow.

register --> onCancelled

Depending on when onCancelled callbacks are invoked, onCancellationRequested would then be a better name, to match isCancellationRequested.

from cancellationtoken.

OK, I'm back to looking at this. Your points seem good to me, @conradreuter. I'll follow up with an updated API proposal shortly.

from cancellationtoken.

Here's the updated API. Feedback?

/**
 * A token that can be passed around to inform consumers of the token that a
 * certain operation has been cancelled.
 */
class CancellationToken {
  /**
   * A cancellation token that is already cancelled.
   */
  public static readonly CANCELLED: CancellationToken;

  /**
   * A cancellation token that is never cancelled.
   */
  public static readonly CONTINUE: CancellationToken;

  /**
   * Gets a value indicating whether the token has been cancelled.
   */
  public get isCancellationRequested(): boolean {
    return this.isCancelled;
  }

  /**
   * Gets the reason why this token has been cancelled.
   */
  public get reason(): any {
    throw new Error("not yet implemented.");
  }

  /**
   * Returns a promise that resolves when some operation resolves,
   * or rejects when the operation is rejected or this token is canceled.
   */
  public or<T>(operation: Promise<T>): Promise<T> {
    throw new Error("not yet implemented.");
  }

  /**
   * Throw a {CancellationToken.CancellationError} instance if this token is cancelled.
   */
  public throwIfCancelled(): void {
    throw new Error("not yet implemented.");
  }

  /**
   * Requests a callback when cancellation occurs.
   * If this token is already cancelled, the callback is invoked before returning.
   * @param cb The method to invoke when cancellation occurs, with the reason.
   * @returns A method that revokes the request for notification.
   */
  public onCancellationRequested(cb: (reason?: any) => void): () => void {
    throw new Error("not yet implemented.");
  }

  /**
   * Whether the token can be cancelled.
   */
  public readonly canBeCancelled: boolean;

  private constructor(private isCancelled: boolean, canBeCancelled: boolean) {
    this.canBeCancelled = canBeCancelled;
  }

  /**
   * Creates a {CancellationToken} and a method that can cancel it.
   * @returns The token, and a function that cancels it.
   */
  public static create(): { token: CancellationToken, cancel: (reason?: any) => void } {
    throw new Error("not yet implemented.");
  }

  /**
   * Create a {CancellationToken} that is cancelled when all of the given tokens are cancelled.
   *
   * This is like {Promise<T>.all} for {CancellationToken}s.
   *
   * @param tokens The tokens that the new token depends on.
   * @returns a token that depends on the given tokens.
   */
  public static all(...tokens: CancellationToken[]): CancellationToken {
    throw new Error("not yet implemented.");
  }

  /**
   * Create a {CancellationToken} that is cancelled when at least one of the given tokens is cancelled.
   *
   * This is like {Promise<T>.race} for {CancellationToken}s.
   *
   * @param tokens The tokens that the new token depends on.
   * @returns a token that depends on the given tokens.
   */
  public static race(...tokens: CancellationToken[]): CancellationToken {
    throw new Error("not yet implemented.");
  }
}

module CancellationToken {
  /**
   * The error that is thrown when a {CancellationToken} has been cancelled and a
   * consumer of the token calls {CancellationToken.throwIfCancelled} on it.
   */
  export class CancellationError extends Error {

    public constructor(

      /**
       * The reason why the token was cancelled.
       */
      public readonly reason: any
    ) {
      super(`Operation cancelled (${JSON.stringify(reason)})`) /* istanbul ignore next: see https://github.com/gotwarlost/istanbul/issues/690 */
      Object.setPrototypeOf(this, CancellationError.prototype)
    }
  }
}

export default CancellationToken

from cancellationtoken.

I like the API redesign overall. Some minor issues:

1. For consistency throwIfCancelled should be renamed to throwIfCancellationRequested.
2. I don't really see the point in canBeCancelled. I would just remove it. Do you see a use-case?
3. The name or is a bit too clever for my taste. I like race more, because it communicates the time aspect. However, I see a potential clash with the static race method.

What do you think?

from cancellationtoken.

For consistency throwIfCancelled should be renamed to throwIfCancellationRequested.

Done. Although at this point, with all the long method names, and no precedent like .NET has for a canceled task/promise, I wonder if we should just truncate all of them as we considered earlier and just consider a token canceled rather than a signal of a cancellation request. I'm open either way.

I don't really see the point in canBeCancelled. I would just remove it. Do you see a use-case?

Absolutely. Knowing whether the caller might cancel allows code to make more efficient decisions when cancellation isn't a possibility. We already optimize using this in the all and race methods, for example. In all my .NET years of programming, I've very rarely used it though. So if you'd rather, we could make it a private property until someone says they need it publicly. The private property will allow all and race to continue working. But IMO, leaving it public won't ever hurt us, since the property must be there to support internal requirements, and it's there publicly in .NET perhaps for reasons I'm not aware of. So my vote is we leave it public.

The name or is a bit too clever for my taste. I like race more, because it communicates the time aspect. However, I see a potential clash with the static race method.

😆 Ya, I wondered what the reaction would be to that one. My experience with TypeScript/Javascript pop terminology is very shallow, so I'm flexible here. How about racePromise?

from cancellationtoken.

For 3 my colleague had an interesting idea. We could just get rid of or and redefine the static race function.

/**
 * Create a {Promise} whose outcome is determined by whatever happens first:
 * - When one of the promises is resolved, the {Promise} is resolved.
 * - When one of the promises is rejected, the {Promise} is rejected.
 * - When one of the tokens is cancelled, the {Promise} is rejected.
 *
 * This is like {Promise<T>.race} for {CancellationToken}s.
 */
public static race<T>(...promisesAndTokens: (Promise<T> | CancellationToken)[]): Promise<T> {
  throw new Error("not yet implemented.");
}

all can then be defined accordingly.

I am not sure if I really like this approach, but I thought it is worth sharing and discussing.

from cancellationtoken.

That sounds good to me. But IMO it does not makes sense for all to be implemented similarly. I can't think of a use case where anyone would use all and mix tokens and promises together.

from cancellationtoken.

Ooh, that means that racing two cancellation tokens becomes a promise instead of another token. Not a fan, actually. We could try an overload strategy perhaps

from cancellationtoken.

hmmmm... but overloads are messy beasts. I'd prefer racePromise or some other name, I guess.

from cancellationtoken.

Done. Please check #2 for update.

from cancellationtoken.

I made some comments, but as the interface is stable now, I will merge it and make the changes on my own. (-:

from cancellationtoken.

Great. It was a pleasure working with you. I look forward to an updated NPM package.
Which, BTW, probably should be bumped to 2.0.

from cancellationtoken.