Fix spin-loop/cleanup failure mode within run loop #42
Conversation
This ensures that exceptions raised in thread callback hooks are rescued and properly mark jobs as failed. Fixes Betterment#23 and Betterment#41
(like in pre-threading implementation)
| pool = Concurrent::FixedThreadPool.new(jobs.length) | ||
| jobs.each do |job| | ||
| pool.post do | ||
| run_thread_callbacks(job) do |
There was a problem hiding this comment.
The explanation might make more sense if I put it here next to the related code.
Previously, if run_thread_callbacks crashed for any reason, neither success nor failure would be incremented. Furthermore, the job cleanup (down at the end of run(job)) would never occur, so attempts would not be incremented and run_at would not be bumped, making the job immediately available for pickup again.
This is what I mean by the "spinloop". A job gets picked up, its thread crashes, no cleanup occurs, it gets immediately picked up again, etc. Pushing the run_thread_callbacks call into run_job allows us to clean up the job if, e.g., it fails to deserialize, or if one of our callbacks fails to connect to a secondary resource (as was the case in #41).
| true # did work | ||
| rescue DeserializationError => e | ||
| job_say job, "FAILED permanently with #{e.class.name}: #{e.message}", 'error' | ||
|
|
||
| job.error = e | ||
| failed(job) | ||
| false # work failed |
There was a problem hiding this comment.
This was a bug that improperly reported deserialization errors in the success number, which only impacted the logging output.
| @@ -8,7 +8,7 @@ jobs: | |||
| strategy: | |||
| fail-fast: false | |||
| matrix: | |||
| ruby: ['2.6', '2.7', '3.0', '3.1', '3.2'] | |||
| ruby: ['2.7', '3.0', '3.1', '3.2'] | |||
There was a problem hiding this comment.
we can make this more formal, but I wanted to unblock this build for now, without the linter churn that comes from actually changing the min supported Ruby.
|
|
||
| num.times do | ||
| while total < num |
There was a problem hiding this comment.
So I'm not sure how likely it is to matter but this change means that we will keep looping until we see num jobs which means that if the queue becomes empty before we hit num we will keep looping, right?
Is that okay or desired?
There was a problem hiding this comment.
There's a break if empty? below that covers that case as well, so we should only ever continue the loop if there are jobs being returned in the query (and that's consistent with the way it worked pre-threading too).
There was a problem hiding this comment.
ah! i missed that. excellent.
This ensures that exceptions raised in thread callback hooks are rescued and properly mark jobs as failed.
This is also a good opportunity to change the
numargument (ofwork_off(num)) to mean number of jobs (give or take a few due tomax_claims), not number of iterations. Previously (before threading was introduced) I think it meant number of jobs (though jobs and iterations were 1:1). I would not have done this before the refactor, because there was no guarantee that one ofsuccessorfailurewould be incremented (the thread might crash for many reasons). Now, we only incrementsuccessand treattotal - successas the "failure" number when we return from the method.Fixes #23 and #41
This is also a prereq for a resolution I'm cooking up for #36