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

No createTimer if TimerService disposed (STANFORD-65) #241

Merged
merged 2 commits into from
Dec 15, 2023
Merged

Conversation

mpqmpqm
Copy link
Contributor

@mpqmpqm mpqmpqm commented Dec 13, 2023

Pair with Stanford SMSVP PR.

Each of the two calls to createTimer in TimerService is gated behind at least one asynchronous request, either TimerService.getCurrentTime or TimerService.getState. TimerService.cancel doesn't know about either of these async pipelines. That means you can write ...

const timerService = new TimerService();
const channel = timerService.getChannel(); // initiate `TimerService.getState()`
timerService.cancel();

... and it's possible that timerService.cancel() will finish before createTimer is called.

This is a problem for the following real code:

// client code
class TimerHelper {
    constructor(options = { scope: 'user', tickInterval: ONE_SECOND }) {
        this.timerService = new F.service.Timer(options);
    }

    restartTimer(timeLimit, tickHandler) {
        const options = this.timerService.options;
        return this.cancelTimer(options).then(() => {
            return this.autoStartTimer(timeLimit, tickHandler);
        });
    }

    autoStartTimer(timeLimit, tickHandler) {
        return this.timerService.autoStart({ timeLimit })
            .then((initialTime) => {
                const channel = this.timerService.getChannel();
                channel.subscribe(this.timerService.ACTIONS.TICK, tickHandler);
                return initialTime;
            });
    }

    cancelTimer(options = { scope: 'user', tickInterval: ONE_SECOND }) {
        return this.timerService.cancel().catch((err) => {
            if (err.status !== 404) throw err;
            return Promise.resolve();
        }).then(() => this.timerService = new F.service.Timer(options));
    }
}

When you call TimerHelper.restartTimer ...

  1. cancel the current this.timerService
  2. Reassign this.timerService
  3. autoStart on the new this.timerService
  4. getChannel on the new this.timerService

When you call TimerHelper.restartTimer many times quickly (going from step 4 to 1), the later-initiated cancel call may finish before the first-initiated getChannel. Since the interval is created at the end of getChannel and cleared during cancel, you end up with a dangling interval and no reference to clean it up.

So we simply dispose at the beginning of cancelTimer ...

// client code
cancelTimer(options = { scope: 'user', tickInterval: ONE_SECOND }) {
    this.timerService.dispose(); // changed line
    return this.timerService.cancel().catch((err) => {
        if (err.status !== 404) {
            throw err;
        }
        return Promise.resolve();
    }).then(() => this.timerService = new F.service.Timer(options));
}

... and if the TimerService has been disposed by the time it's time to start the interval, do nothing:

// libs code
function createTimer(actions, currentTime) {
    if (me.interval || !merged.tickInterval || me.disposed) {
        return;
    }
    ...
}

@mpqmpqm mpqmpqm requested a review from safetybelt December 13, 2023 19:33
@mpqmpqm mpqmpqm merged commit 8d5c23b into master Dec 15, 2023
@mpqmpqm mpqmpqm deleted the STANFORD-65 branch December 15, 2023 16:50
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