https://pantsbuild.org/ logo
#development
Title
# development
h

hundreds-breakfast-49010

07/23/2020, 7:57 PM
I think I've identified an issue with
async_semaphor
https://github.com/pantsbuild/pants/blob/master/src/rust/engine/async_semaphore/src/lib.rs#L103 <- whenever we drop a
Permit
, we increment
available_permits
, which is an integer, but we only look at the front item in the
waiters
queue
so if we have a case where we spawn a bunch of processes on a
BoundedCommandRunner
all at once, more than the limit
when the first
n
processes finish, they'll increment the available count but call
wake_by_ref
on the same waiting process
so if a batch of processes finishes at roughly the same time, they'll only wake up one waiting process, not
n
waiting processes
this may or may not be what's going on with the slot ID bug being discussed in #getting-started but it seems incorrect to me
@witty-crayon-22786 do you have any thoughts?
w

witty-crayon-22786

07/23/2020, 8:15 PM
… we should switch to tokio semaphore if possible.
and then implement the “slot” feature some other way.
h

hundreds-breakfast-49010

07/23/2020, 8:16 PM
that's probably a good long-term idea
w

witty-crayon-22786

07/23/2020, 8:17 PM
i need to work on other things, but if the gist of the above is that the semaphore is buggy, then i would strongly endorse switching. because it isn’t the first time
tokio’s semaphore didn’t exist when I wrote that.
h

hundreds-breakfast-49010

07/23/2020, 8:18 PM
definitely makes sense (long-term) to use a common library functionality instead of bespoke code
w

witty-crayon-22786

07/23/2020, 8:18 PM
re: slots: could literally do it with integers in a Stack/Vec under a mutex.
h

hundreds-breakfast-49010

07/23/2020, 8:19 PM
https://github.com/pantsbuild/pants/pull/10436 might fix things more quickly for the person who's having issues today
w

witty-crayon-22786

07/23/2020, 8:19 PM
i mean, as i said: if the semaphore is buggy now, let’s switch. but yea, thanks for investigating.
@hundreds-breakfast-49010: we’re probably not going to do a release today, so maybe keep that fix green, and then if you’re still happy with it as we ramp up to a release, we can land it.
or whatever
just lemme know.
h

hundreds-breakfast-49010

07/23/2020, 8:22 PM
the above draft PR added a new, more comprehensive test for sane concurrency ID numbers in
async_semaphor
, which only passes if I make that adjustment to the call to
wake_by_ref
in the drop impl
so I'm hoping that this will solve that person's problem and if so we should merge it soon-ish. I do think it makes sense to stop using custom semaphor code, but unless it's really easy to swap it out I think that's better off as a post-2.0 thing
w

witty-crayon-22786

07/23/2020, 8:33 PM
we need to independently discuss when we’re going to do the next release. i’ll be able to discuss that starting in a few hours
so i’m going to snooze this conversation for a few hours, and then we land that and cut another release, or land any other fix.
h

hundreds-breakfast-49010

07/23/2020, 8:34 PM
kk
w

witty-crayon-22786

07/23/2020, 8:34 PM
@fast-nail-55400 would also be a good reviewer for that change, probably.
👍 1
h

happy-kitchen-89482

07/23/2020, 10:53 PM
Looking at it now, I don't know enough Rust to review the code in any depth, but I can reason about concurrency.
h

hundreds-breakfast-49010

07/23/2020, 11:06 PM
experimenting with this code more on my dev laptop, I don't think this actually solves the problem
I think we actually need to explicitly keep a set of used concurrency IDs in the
Inner
data structure, and I have some changes locally that do this and are passing tests
so I'll update that PR in a bit
(basically, if we use the value of
available_permits
as an effective id, once enough tasks are scheduled to exhaust the ids, we end up assigning an id of
1
whenever a particular task finishes. so we count down from whatever the value of local concurrency is to 1, and then only assign 1 to processes thereafter. instead we should give a newly-scheduled process the id of whatever process just finished, which means we need store those ids in a separate data structure, which is easy enough to do)
h

happy-kitchen-89482

07/23/2020, 11:13 PM
Yeah, looking at this I wasn't able to prove to my own satisfaction that this solves the problem.
h

hundreds-breakfast-49010

07/23/2020, 11:15 PM
with the changes I have locally right now, I can log the concurrency id number given to processes in our own tests, and now I'm seeing a wide variety of different numbers, instead of just 1 all the time after counting down from 16 (I have an 8 core CPU)
w

witty-crayon-22786

07/23/2020, 11:20 PM
i still haven’t been able to look at this deeply, but if the fix is not trivial, i would continue to recommend switching to the tokio semaphore, and implementing the slot separately with a
Mutex<Vec<usize>>
full of the number of slots you want.
f

fast-nail-55400

07/23/2020, 11:23 PM
I’m looked at the PR but I’m not well versed enough in synchronization stuff to opine on it in any way I can be sure of
h

hundreds-breakfast-49010

07/23/2020, 11:37 PM
w

witty-crayon-22786

07/24/2020, 12:46 AM
ok, sorry… out of my hole now
will take a look.
@hundreds-breakfast-49010: commented: thanks