Max recursion async #58
Conversation
If the Async recursion level goes past the max_depth. Then it fails silently.
Add test_max_depth which ensures that when an Async reaches past the max_depth, then it will cease and an info log will be put out.
When the job is executing, the current_depth option is incremented. Add test to ensure incrementing works.
When a new Async is being started as the result of a finished Async, or in the case of an Async restarting, the options will be updated with the correct next_depth. Tests have been updated and added so that both cases of a new Async started and an Async being restarted, the increment is checked.
Ensure that when a custom max_depth is set, that it is propagated onwards down the chain through either the restarted Async's options or the new Async's options. Add tests to cover both new async start and restarted async.
Move max and current depth values into a single _recursion dict with the Async options. Update tests to reflect this change. Change max depth reached logging message to warning level.
The max recursion check code is being moved into Async and out of the processors run_job() function.
Async._check_and_update_depth() is a method that will update this Async's current depth and max depth from an executing Async. It is called from within the start() method to determine if max recursion depth has been reached.
mock out _check_and_update on the tests for start() as the depth is not what those functions are testing. This prevents a problem with _check_and_update_depth from invalidating those tests.
Tweak dict handling in _check_and_update_depth and add tests for _check_and_updated_depth
|
@robertkluin-wf @beaulyddon-wf @ericolson-wf |
|
+1 |
| @@ -309,6 +318,40 @@ def _restart(self): | |||
|
|
|||
| return self.start() | |||
|
|
|||
| def _check_and_update_depth(self): | |||
There was a problem hiding this comment.
I feel like this function could be named better.
|
I feel like we also need to expose a "nice" way to reset the current depth. Maybe I am wrong on that though? |
It is unnecessary to mock _check_recursion_level in the tests so they were removed. Changed tests to use new _check_recursion_level name
Use get_current_async to get the executing async instead of get_local_context. If an interior Async inside a chain sets a new custom max_depth, ensure that value is propagated instead of the existing cusomt depth. Add test to cover the scenario of a new interior custom max_depth.
When the max recursion depth has been reached, an async.Abort exception is raised. Update tests to comply with Abort exception.
|
I'm gonna copy this comment I made on one of robert's comments up here as I think we need to discuss it (and it's hidden now by other changes): @robertkluin-wf I think you're right about raising an Abort. That's a good way to use our preexisting capability. As far as where to do this, I'm having a hard time determining where to handle this stuff. If we leave it in If I put some of this stuff in The other option is to split all this logic up into much smaller pieces and spread it throughout Any thoughts? |
Rename _check_recusion_level() to _update_recursion_level and move the call to it from start() to __init__. This way the depth is incremented upon creation of the Async. Move the checking logic out of async and into processor.run_job(). Now it is predictable when the current_depth is reached and the Abort is thrown.
Move decrement operation to to_dict. Update all the tests to accomodate new max depth changes.
|
@robertkluin-wf @beaulyddon-wf @ericolson-wf @johnlockwood-wf I went back and reconsidered some of this. Take another look and see what you think. |
Rather than incrementing the recursion level at instantiation time, perform the recursion level check and increment when the async is actually converted to a task. This will simplify a lot of management around handling the recursion increment and check.
If an async has been inserted, we want to run it. Blocking execution of the task at run time would appear as a 'silent' failure. Rather, we would prefer raise this error in advance so that the developer has a chance to handle the issue.
Instead of maintaining the recursion level in init and to_dict, moved the recursion level increment to the to_task method. This should simplify the maintenance of the recursion depth counter.
The check recursion depth method is called from to_task, and will raise an AsyncRecursionError if the task is too deeply nested.
Max recursion async
|
@robertkluin-wf @beaulyddon-wf @ericolson-wf I think this is now the final state of this code. Anyone have any more thoughts on it? |
|
I think this is as good as it will get for now. A lot of this feels really hacky, but if @ericolson-wf or @beaulyddon-wf can't think up something better then I'll merge it in. |
|
looks ok to me |
ghost
merged commit 
Here is the implementation for #41
This is a rewritten version of #52
After considering the comments in the last PR, I rewrote this using a slightly different tactic, checking everything on the
Asyncside instead of within therun_job()function.