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

TimeoutStrategy.Agressive causes TimeoutPolicy to time out immediately #16

Closed
ekillops opened this issue Feb 24, 2020 · 4 comments
Closed
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@ekillops
Copy link
Contributor

ekillops commented Feb 24, 2020

TimeoutPolicy.ts (39)

  public async execute<T>(fn: (context: ICancellationContext) => PromiseLike<T> | T): Promise<T> {
    const cts = new CancellationTokenSource();
    const timer = setTimeout(() => cts.cancel(), this.duration);

    try {
      if (this.strategy === TimeoutStrategy.Cooperative) {
        return await fn({ cancellation: cts.token });
      }

      return Promise.race<T>([
        fn({ cancellation: cts.token }),
        cts.token.cancellation(cts.token).then(() => {
          throw new TaskCancelledError(`Operation timed out after ${this.duration}ms`);
        }),
      ]);
    } finally {
      cts.cancel();
      clearTimeout(timer);
    }
  }

When using TimeoutStrategy.Aggressive, the Promise.race call is not await-ed causing the finally block to execute immediately, resulting in cts.cancel() being called before the timer runs out.

In my testing, adding an await before the Promise.race call results in the expected behavior of the cancellation being triggered from within the setTimeout call.

@connor4312
Copy link
Owner

connor4312 commented Feb 24, 2020

Good catch! Feel free to PR, otherwise I can do so soon.

@connor4312 connor4312 added bug Something isn't working good first issue Good for newcomers labels Feb 24, 2020
@ekillops
Copy link
Contributor Author

PR submitted.

@connor4312
Copy link
Owner

Thanks for the contribution, published in 0.1.4 ✨

@ekillops
Copy link
Contributor Author

Sweet! Thanks for the fast response.

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

No branches or pull requests

2 participants