-
-
Notifications
You must be signed in to change notification settings - Fork 129
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
Conversation
There was a problem hiding this 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 👍
There was a problem hiding this 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.
if (isClosing) { | ||
// Skip processing this data and exit. | ||
return; | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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…
There was a problem hiding this comment.
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 😉
There was a problem hiding this comment.
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.
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. |
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
yarn lint:fix
passes.yarn test
passes.RELEASE_NOTES.md
file (if one exists).