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

Flag to stop LDS loop while no timer is active #756

Closed
wants to merge 3 commits into from

Conversation

ab-pm
Copy link
Contributor

@ab-pm ab-pm commented Sep 10, 2021

Fixes #755.

I hope using the duration variable for the flag is not "too clever", but I wanted to get around writing if (!closed) twice.

Checklist

  • My code matches the project's code style and yarn lint:fix passes.
  • I've added tests for the new feature, and yarn test passes.
  • I have detailed the new feature in the relevant documentation.
  • I have added this feature to 'Pending' in the RELEASE_NOTES.md file (if one exists).
  • If this is a breaking change I've explained why.

Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets use -1 rather than 0 as the special meaning value for sleep duration but otherwise looks okay from a quick review 👍

packages/lds/src/index.ts Outdated Show resolved Hide resolved
packages/lds/src/index.ts Outdated Show resolved Hide resolved
packages/lds/src/index.ts Outdated Show resolved Hide resolved
packages/lds/src/index.ts Outdated Show resolved Hide resolved
packages/lds/src/index.ts Outdated Show resolved Hide resolved
packages/lds/src/index.ts Show resolved Hide resolved
Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided overloading nextSleepDuration was too clever in the end - namely that you have to track to see if the * 10 was applied multiple times/etc. Much more straightforward if the mutable value is just a boolean.

I've also added an early exit after receiving but before processing the data.

packages/lds/src/index.ts Outdated Show resolved Hide resolved
packages/lds/src/index.ts Outdated Show resolved Hide resolved
packages/lds/src/index.ts Outdated Show resolved Hide resolved
packages/lds/src/index.ts Show resolved Hide resolved
packages/lds/src/index.ts Outdated Show resolved Hide resolved
packages/lds/src/index.ts Show resolved Hide resolved
if (isClosing) {
// Skip processing this data and exit.
return;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the main part I need to think about if it's acceptable or not - it's effectively throwing away LDS data.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't throw it away. I don't think there's anything wrong with firing a few other events, even if they don't reach anyone. Better than discarding data that would have been needed.

OTOH, teardown is only available in test environments, so it might not matter at all… Or is ldSubscription.close() also used for graceful exit in prod?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm not happy with it... but also calling stuff after release resolves definitely feels wrong. I'm thinking we should make the release method await the loop exiting, but that's a bit more code than I can write via the GitHub interface 😆 Fancy taking it on? I was thinking wrapping loop in a promise we can await might work. Or maybe using a deferred: https://github.com/graphile/worker/blob/main/src/deferred.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a let onClose: (() => void) | null instead of the isClosing flag, with return new Promise(resolve => { onClose = resolve; }) might work? I'm only using Github here either, that's why I failed the prettier check…

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah one of us should probably check this out and actually write it in an editor. And... you know... see if it works 😉

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just spent some time thinking this over and I feel the above 4 lines should be removed and a mechanism to wait for the current loop() call to exit before ldSubscription.close() calls await client.close() should be added.

@benjie
Copy link
Member

benjie commented Oct 5, 2023

It seems no-one was sufficiently motivated to check this out locally and do the required editing over the past couple years, so I'm going to close it.

@benjie benjie closed this Oct 5, 2023
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.

Race condition when closing LDS
2 participants