Fix out-of-bounds panic in progress printer#378
Conversation
We ran into an out-of-bounds panic when we had more tasks reporting as "currently running" than the number of status bars we had. That was due to a lack of defensive checks in the face of a possible race condition. The race condition leads the `PrintStatuses` method seeing more "currently running" tasks than there can technically be. The race condition is between the `PrintStatus` method that prints the slices of pointers to `*TaskStatus` and the tasks themselves updating the status non-atomatically. At least that's what it seems like. I'm not 100% sure about the cause yet, but I could reproduce the issue by simply injecting more statuses at the top of `PrintStatuses`. The code here fixes it.
eseliger
left a comment
There was a problem hiding this comment.
Seems a little hacky but I don't know another simple way to get it in on a Friday evening, so LGTM
LawnGnome
left a comment
There was a problem hiding this comment.
Yeah, I also don't understand exactly how this happened, but as you say, let's get the defensive check in regardless.
|
HA! Yesterday, in the shower, I figured out how it happens! It's a race condition between the So what happens is this. Say you have tasks A, B, C, D. A and B are currently running.
A solution would be a lock around the |
* Fix out-of-bounds panic in progress printer We ran into an out-of-bounds panic when we had more tasks reporting as "currently running" than the number of status bars we had. That was due to a lack of defensive checks in the face of a possible race condition. The race condition leads the `PrintStatuses` method seeing more "currently running" tasks than there can technically be. The race condition is between the `PrintStatus` method that prints the slices of pointers to `*TaskStatus` and the tasks themselves updating the status non-atomatically. At least that's what it seems like. I'm not 100% sure about the cause yet, but I could reproduce the issue by simply injecting more statuses at the top of `PrintStatuses`. The code here fixes it. * Add changelog entry
We ran into an out-of-bounds panic when we had more tasks reporting as
"currently running" than the number of status bars we had.
That was due to a lack of defensive checks in the face of a possible
race condition.
The race condition leads the
PrintStatusesmethod seeing more"currently running" tasks than there can technically be.
The race condition is between the
PrintStatusmethod that prints theslices of pointers to
*TaskStatusand the tasks themselves updatingthe status non-atomatically.
At least that's what it seems like. I'm not 100% sure about the cause
yet, but I could reproduce the issue by simply injecting more statuses
at the top of
PrintStatuses. The code here fixes it.I'm still trying to find out exactly why it happens, but I do think this code should be in there in any case, that's why I'm opening up this PR.