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.

Sign up for GitHub

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

Jump to bottom

fix: prevent event delivery race condition #232

Closed
wants to merge 8 commits into from
Closed

fix: prevent event delivery race condition #232

wants to merge 8 commits into from

Conversation

skeggse
Copy link
Member

@skeggse skeggse commented Apr 16, 2020

This solution introduces a new client-specific identifier that can be used independent of the normal identifier. This approach permits the continued use of the global incrementing queue id so that we don't break existing requirements around the use of that id for reporting and metrics.

Fixes #189, adds tests to fix #22.

@skeggse skeggse added the bug label Apr 16, 2020
@skeggse skeggse added this to the 1.2.4 milestone Apr 16, 2020
@skeggse skeggse requested a review from hughsw April 16, 2020 23:53
@skeggse skeggse self-assigned this Apr 16, 2020
@skeggse skeggse linked an issue Apr 16, 2020 that may be closed by this pull request
'success' event not fired #78
Closed
@skeggse
Copy link
Member Author

skeggse commented Apr 16, 2020

This is my proposal for fixing the behavior seen in #189 and #78, as it solves some of my concerns per #197 (comment). It builds upon both #193 and the discussion in #197

@skeggse skeggse force-pushed the fix-event-race-condition branch 2 times, most recently from 30dd67c to a9dd966 Compare April 17, 2020 00:30
@coveralls

This comment has been minimized.

Sign in to view
@coveralls
Copy link

coveralls commented Apr 17, 2020 •
edited

Coverage Status

Coverage remained the same at 100.0% when pulling e280567 on fix-event-race-condition into 26edf31 on master.

@skeggse skeggse force-pushed the fix-event-race-condition branch 3 times, most recently from 61d2db0 to 9fa69a4 Compare April 17, 2020 19:25
Eli Skeggs added 8 commits April 17, 2020 12:39
fix: prevent event delivery race condition
deb1eec 
This solution introduces a new client-specific identifier that can be used independent of the
normal identifier. This approach permits the continued use of the global incrementing queue `id`
so that we don't break existing requirements around the use of that `id` for reporting and metrics.
fix: do not assign local job ID for job with ID
e149af8
test: use correct function for delay
13772bf
test: fix race condition in immediate removal test
9e17689
fix: add missing error handler
9778d7f
fix: handle exceptions from event handlers
64323b2
refactor: avoid deferred
65faaa4
test: use waitMethod for readability
e280567
@skeggse skeggse mentioned this pull request Apr 20, 2020
Open
@hughsw
Copy link
Collaborator

hughsw commented Apr 20, 2020

This seems like multiple PRs, with improved error handling, new tests?, fixing a bug, and client UUID support mingled together. Being new to the code base it will take me some time to separate the concerns and their implementations.

The improved error handling seems like a good thing. At first blush the client UUID support seems heavy, lots of new code and conditionals -- are there requests for this?

@skeggse
Copy link
Member Author

skeggse commented Apr 20, 2020

This seems like multiple PRs

That's...a running problem for me. I'll look at splitting this up into more digestible PRs. Sometimes I start working on stuff and the scope just keep creeping. Sorry about that!

At first blush the client UUID support seems heavy, lots of new code and conditionals -- are there requests for this?

It's specifically to maintain feature parity for our internal tooling we've built around bee-queue, yeah.

@hughsw
Copy link
Collaborator

hughsw commented Apr 20, 2020

Yeah, I'm familiar with the feature creep on a dev branch.

Can you say more about "feature parity" details that need to be maintained. I can think of three:

  1. Queue#checkHealth().newestJob counts the number of default-id jobs
  2. Job.save() makes a single Redis call (or, preserve benchmark performance)
  3. job.id have a lexical ordering such that job IDs sort according to the order in which they were saved

And of course, for the bug (#78) to be fixed, all successfully saved jobs will emit exactly one of 'succeeded' or 'failed' event

Are there other features or semantics that need to be preserved or have been requested?

Of the three PRs approaching these matters:

Does this line of questioning and feature factoring make sense?

@skeggse skeggse modified the milestones: 1.2.4, 1.3.0, 1.3.1 Nov 3, 2020
@skeggse skeggse modified the milestones: 1.3.1, 1.3.2 Nov 4, 2020
@compwright compwright closed this Nov 23, 2022
@skeggse skeggse deleted the fix-event-race-condition branch November 23, 2022 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hughsw hughsw Awaiting requested review from hughsw

Assignees

@skeggse skeggse

Labels
bug
Projects
None yet
Milestone
1.3.2
4 participants
@skeggse @coveralls @hughsw @compwright