https://pantsbuild.org/ logo
Join Slack
Powered by
are there good examples in the pants source of wra...
# development
f
are there good examples in the pants source of wrapping some code in a workunit that someone could point me at?
Is the basic pattern to obtain a WorkunitStore via 
context.workunit_store
and then use 
start_workunit
and 
complete_workunit
?
w
cc @hundreds-breakfast-49010
f
I think i figured it out. sending PR.
h
@fast-nail-55400 yeah, look at where we use the 
with_workunits
function defined in 
workunit_store/src/lib.rs
f
(the PR doesn’t use that 
with_workunits
helper. Should it?)
h
I"m looking. but I don't think it needs to
there's other code in nodes.rs that avoids it
f
I assume that 
scope_task_workunit_state
is to scope panics?
h
it's something pertaining to using workunits in futures contexts that I don't fully understand. @witty-crayon-22786 added that code
w
task
here is the rust async equivalent of a thread
in particular, a 
task
is a tokio task.
that scope call is to propagate the “parent” workunit information along, so that if we create child workunits, they have parents.
so… yes, it should be used if there is a possibility that you’re “expecting to have children” (heh)
h
this would be good to add as documentation comments in the workunit crate
w
it’s early days for this. we could use better syntax.
@hundreds-breakfast-49010: it’s there, probably…
h
I think there's definitely some more polish we could add there
f
seems like I should use with_workunit though and not use start_workunit/complete_workunit directly as I did in the PR
👍 1
w
@hundreds-breakfast-49010: …yea, nevermind. there isn’t much explanation in there. sorry about that.
that’s the safe bet, yea.
h
@fast-nail-55400 yeah. I think that would be a reasonable refactor
ideally we would only expose 
with_workunit
. there's only one other place in the code where we actually create workunits without using that helper, in node.rs
➕ 1
w
do we need to though?
h
well it's not strictly necessary; in the sense that the code works as it. but it is confusing that the workunit create exposes two slightly different APIs for doing the same thing
w
… yea, i guess it does.
h
in the only two places we actually define workunits (before tom's commit anyway)
w
@hundreds-breakfast-49010: the nodes.rs stuff needs to add metadata to the workunit i think, which you’d need an api for in 
with_workunit
. but yea, could call it a low-level API.
h
anyway I think this is polish, and not polish that needs to be prioritized right now. that said @fast-nail-55400 if you want to refactor your PR to using 
with_workunit
, you might as well
w
ok, making progress on this other thing: checking back out of this thread. sorry for not adding docs when i was in there =(
h
although I would've ack'd it regardless
w
(ping again if need be!)
h
no worries @witty-crayon-22786 I could've also added more documentation when I've poked around in that code
f
with_workunit consumes the WorkunitStore which means I can’t use it more than once?
or do I need to clone the WorkunitStore?
h
it's cheaply-clonable
the struct defines a single boolean and an 
inner
member wrapped in an 
Arc<Mutex<_>>
so just calling 
.clone()
on a 
WorkunitStore
is a (relatively) cheap operation. the cost comes from interacting with the 
Arc<Mutex<_>>
when you call methods on 
WorkunitStore
that get proxied to 
inner
f
👍
Open in Slack
PreviousNext
Powered by