https://pantsbuild.org/ logo
Join Slack
Powered by
Is there a better way to solve this than using `Bo...
# development
h
Is there a better way to solve this than using 
Box
? I wrote this function signature:
Copy code
pub async fn with_workunit<F>(
  workunit_store: WorkunitStore,
  name: String,
  initial_metadata: WorkunitMetadata,
  future_fn: Box<dyn FnOnce(&mut Workunit) -> F>,
) -> F::Output
where
  F: Future,
{
In contrast, the old function has:
Copy code
final_metadata_fn: for<'a> FnOnce(&'a F::Output, WorkunitMetadata) -> WorkunitMetadata
But when I try to do that, I get
Copy code
error[E0277]: the size for values of type `(dyn for<'a, 'r> FnOnce(&'a &'r mut Workunit) -> F + 'static)` cannot be known at compilation time
   --> workunit_store/src/lib.rs:821:3
    |
821 |   future_fn: for<'a> FnOnce(&'a &mut Workunit) -> F,
    |   ^^^^^^^^^ doesn't have a size known at compile-time
h
what's the exact signature of the version of the function you're trying when you get that compilation error?
h
Copy code
pub async fn with_workunit<F>(
  workunit_store: WorkunitStore,
  name: String,
  initial_metadata: WorkunitMetadata,
  future_fn: for<'a> FnOnce(&'a &mut Workunit) -> F,
) -> F::Output
where
  F: Future,
Also rip, my current version with 
Box<dyn>
doesn't work:
Copy code
`dyn for<'r> FnOnce(&'r mut Workunit) -> impl futures::Future` cannot be sent between threads safely
This worked šŸ˜Ž
Copy code
pub async fn with_workunit<F>(
  workunit_store: WorkunitStore,
  name: String,
  initial_metadata: WorkunitMetadata,
  future_fn: impl FnOnce(&mut Workunit) -> F,
) -> F::Output
where
  F: Future,
šŸ‘ 1
f
maybe s/String/impl AsRef<str>/ ?
then 
<http://name.as|name.as>_ref().to_owned()
? would be more ergonomic to not have to put 
.to_owned()
at all call sites
w
h
Hm, are there alternatives we could explore?
w
having the closure return a concrete type (i.e., requiring it to be boxed) seems like it might work, but itā€™s more awkward then just returning an arbitrary Future.
šŸ‘ 1
going to see what that looks like. we might have two variants of 
with_workunit
, where only the one that needs access to the workunit requires a boxed future, but.
šŸ‘ 1
oof. yea, this is challenging. i see two ways to do it potentiallyā€¦ the box approach isnā€™t quite working, but seems like it should. the other would be to pass in a non-reference object like 
Fn(InProgressWorkunit) -> Future<T>
which would be dynamically checked. the downside of the dynamic checking is that youā€™d need to panic if somebody leaked the 
InProgressWorkunit
, which is not friendly.
back in a bit.
h
The other would be to pass in a non-reference object likeĀ Fn(InProgressWorkunit) -> Future<T>
This feels more similar to the final_metadata_fn idea, right? As long as we can call 
InProgressWorkunit.increment_counter()
, sounds reasonable
w
re: Box: see the Reddit conversation i linked
re: non-reference, yea: it might be an ok compromise.
and yea, i think that this is something that 
tracing
hasnā€™t quite figure out yet either: https://docs.rs/tracing/0.1.23/tracing/span/struct.Span.html#in-asynchronous-code
actually. no. i think that that syntax would work. itā€™s more error prone though, because outside of 
in_scope
, your parent pointers are wrong.
but i think that we have few enough callsites that it would be workable.
h
the downside of the dynamic checking is that youā€™d need to panic if somebody leaked theĀ InProgressWorkunitĀ , which is not friendly.
This should be deterministic, right? If it's something that us Pants devs will catch when dogfooding Pants day-to-day, I'm less concerned. If it's something we'll easily miss, then hm
should be deterministic
Reasoning about async blocks is tricky
w
This should be deterministic, right? If itā€™s something that us Pants devs will catch when dogfooding Pants day-to-day, Iā€™m less concerned. If itā€™s something weā€™ll easily miss, then hm
yea, thatā€™s what i was thinking. but itā€™s annoying because itā€™s internally a bit magical. going to spend a bit more time on the 
Box
approach, and weā€™ll see.
blugh, frustrating. described here: https://github.com/pantsbuild/pants/issues/11548#issuecomment-777866727, but putting it down for today.
h
Sounds good, thanks Stu!
Open in Slack
PreviousNext
Powered by