Hello I m fixing an issue

Question

Hello I m fixing an issue that RemoteCacheWritesStarted is recorded iff the write finishes I see why the closure is getting canceled before it can finish Ik how to fix but wondering if I should do a more robust fix Specifically < witty crayon 22786> started to fix the issue with workunits and dangling pointers Does it make sense for me to finish that change

hundreds-father-404 · Answer

Note that I m on vacation starting wednesday but looks like this could be a good small scoped change

witty-crayon-22786 · Answer

would the macro fix the issue with the closure or is it a cleanup nearby

witty-crayon-22786 · Answer

a comment describing what you think is happening on 12019 might be good

hundreds-father-404 · Answer

I think it s more adjancent cleanup the fix iiuc is to have a distinct workunit for the remote cache write starting and a distinct one for the remote cache write itself

witty-crayon-22786 · Answer

i would think about it in terms of threads tasks everything that happens in the foreground task should be in one workunit everything in the background should be in another

witty-crayon-22786 · Answer

the effect of the macro is just to use the lifetime of the workunit itself to enforce that it doesn t leak into background work on a different task

witty-crayon-22786 · Answer

so yea the macro would enforce that you don t get that wrong if we want to land it and then incrementally move to it where we can that would work i think i haven t looked at the macro recently but would want to confirm that the behavior of the old method is unchanged

witty-crayon-22786 · Answer

i could also probably clean that up to land today

hundreds-father-404 · Answer

It doesn t look too difficult to do it in one big swoop Want me to take it over

witty-crayon-22786 · Answer

sounds good thanks

average-vr-56795 · Answer

This sounds like it d be a fairly natural use case for an `impl Drop`

hundreds-father-404 · Answer

Ah that makes sense also like a `try finally` in Python land

witty-crayon-22786 · Answer

yea the in progress Workunit struct implements Drop i m not 100 sure how Eric s edgecase works

witty-crayon-22786 · Answer

hundreds-father-404 · Answer

< witty crayon 22786> for the macro should we also fix `context workunit store record observation`

witty-crayon-22786 · Answer

yea probably

average-vr-56795 · Answer

Interesting Yeah I d be interested to know how that `Drop` impl isn t happening

witty-crayon-22786 · Answer

in general i believe that it is workunits are marked canceled when they are dropped

hundreds-father-404 · Answer

gt should we also fix context workunit store record observation Oh hm looks like histograms aren t associated with particular workunits so no need

witty-crayon-22786 · Answer

they sortof are when the workunit completes some data is aggregated into a global structure iirc if all histogram users migrated to this API the internals could be refactored cc < fast nail 55400>

hundreds-father-404 · Answer

Hm bummer <https github com pantsbuild pants pull 11759> does not fix the missing workunit issue But dangling pointers should be fixed at least

witty-crayon-22786 · Answer

thanks for working on that though

witty-crayon-22786 · Answer

might be good to go back and do <https pantsbuild slack com archives C0D7TNJHL p1623096020042100 thread ts=1623095917 041600 amp cid=C0D7TNJHL>

hundreds-father-404 · Answer

Agreed Also in case you missed it I couldn t figure out how to get the `tokio select ` happy and I d love some help if you re free I posted a diff with what I was trying to use

witty-crayon-22786 · Answer

i ll take a look

witty-crayon-22786 · Answer

i expect that it was because the tokio select had an early return sitting in the middle of it and the macro introduces new blocks

witty-crayon-22786 · Answer

still working on it

hundreds-father-404 · Answer

I think so too I m not sure how to avoid that early return tho we need to skip cache writes Maybe return a FallibleProcessResult bool and return outside the macro

witty-crayon-22786 · Answer

coke

hundreds-father-404 · Answer

gt I see why the closure is getting canceled before it can finish This was not true The workunit is not getting dropped because we used `tokio spawn` It should still be running but in a background thread after the Pants session has finished The fix is simple for this add a new workunit that always finishes

witty-crayon-22786 · Answer

sorry for the delay commented on <https github com pantsbuild pants issues 12019 issuecomment 861023968>