Fix risk of dangling workunits by using in_workunit! macro
#11759
Conversation
[ci skip-build-wheels]
…workunit while it runs. [ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
| #[macro_export] | ||
| macro_rules! in_workunit { | ||
| ($workunit_store: expr, $workunit_name: expr, $workunit_metadata: expr, |$workunit: ident| async move { $( $body:tt )* }) => ( | ||
| { | ||
| let (store_handle, mut $workunit) = $crate::_start_workunit($workunit_store, $workunit_name, $workunit_metadata); | ||
| $crate::scope_task_workunit_store_handle(Some(store_handle), async move { | ||
| let result = { | ||
| let $workunit = &mut $workunit; | ||
| async move { $( $body )* } |
This should not add to the stack size in a significant way, afaict... if my understanding is correct then this should only add twoish variables to the stack: the workunit, and the workunit store handle. |
[ci skip-build-wheels]
[ci skip-build-wheels]
[ci skip-build-wheels]
This comment has been minimized.
This comment has been minimized.
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
| let response = with_workunit( | ||
| context.workunit_store.clone(), | ||
| "check_action_cache".to_owned(), | ||
| WorkunitMetadata { | ||
| level: Level::Trace, | ||
| desc: Some(format!("check action cache for {:?}", action_digest)), | ||
| ..WorkunitMetadata::default() | ||
| }, |
There was a problem hiding this comment.
This work unit was moved to remote.rs in check_action_cache, which is where all the counters are incremented
| /// | ||
| /// NB: Public for macro usage: use the `in_workunit!` macro. | ||
| /// | ||
| pub fn _start_workunit( |
There was a problem hiding this comment.
I'm not sure how best to factor the macro with its two arms. For example, perhaps this function should no longer exist and we should inline. We could have one arm call the other for DRY.
There was a problem hiding this comment.
I think that it's ok for now. If it grows at all can DRY it up.
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
| /// | ||
| /// NB: Public for macro usage: use the `in_workunit!` macro. | ||
| /// | ||
| pub fn _start_workunit( |
There was a problem hiding this comment.
I think that it's ok for now. If it grows at all can DRY it up.
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
Eric-Arellano
changed the title
in_workunit! macro
|
FYI I updated the PR title to reflect the change in scope from the original draft PR. |
| cache_lookup_start: Instant, | ||
| action_digest: Digest, | ||
| mut local_execution_future: BoxFuture<'_, Result<FallibleProcessResultWithPlatform, String>>, | ||
| ) -> Result<(FallibleProcessResultWithPlatform, bool), String> { |
There was a problem hiding this comment.
Would it be better to use a tuple struct here so that the bool has a name?
There was a problem hiding this comment.
Change "tuple struct" to just "struct" -- I meant the one that has names ...
Fixes #11548.