Fix race conditions that can lead to a segfault #3667
Conversation
dipinhora
force-pushed
the
raceconditions
branch
from
a7490ee to
efa5393
Compare
Prior to this commit, there could be race conditions with a actor running concurrently on two different threads due to work being done after an actors queue was marked empty. This commit reworks the logic to ensure that the actors queue being marked empty is the last thing that occurs in `ponyint_actor_run` to prevent these race conditions. This commit also adds logic in the cycle detector to not send any messages to actors that are marked as pending destroy to ensure that the cycle detector cannot create a race condition by sending messages to a zombie actor (with `rc == 0`) that is about to be reaped in the near future.
dipinhora
force-pushed
the
raceconditions
branch
from
efa5393 to
3eda728
Compare
| // mark the queue as empty or else destroy will hang | ||
| bool empty = ponyint_messageq_markempty(&actor->q); | ||
|
|
||
| // make sure the queue is actually empty as expected |
There was a problem hiding this comment.
I think we should restate the assumptions you and I have privately spoken about here.
Those points might not be clear to others in the future.
- If no block and rc of 0 then nothing could have sent a message since we did the empty check.
- If not no block, but rc of 0 and cd has never been contacted, nothing could have sent a message since we did the empty check
someone who isn't
src/libponyrt/actor/actor.c
Outdated
| // the invariant that should hold true at this point is that we have | ||
| // not sent the cycle detector a `block` message already | ||
| pony_assert(!has_flag(actor, FLAG_BLOCKED_SENT)); |
There was a problem hiding this comment.
are you sure? i'm not sure this is correct. i think we might have but don't want to again.
otoh, i really need to think about that, and I'm definitely not sure this isn't correct.
There was a problem hiding this comment.
so to get here, CD_CONTACTED would have to be set.
And that means we have sent a block at some point.
There's no guarantee that I see for this that we haven't unblocked and then end up here which is the only way FLAG_BLOCKED_SENT would be set if we get this, right?
In the previous version of this code, this wasn't an assert, it was an if. I think we want an if not an assert here.
SeanTAllen
changed the title
SeanTAllen
added
the
changelog - fixed
|
Hi @dipinhora, The changelog - fixed label was added to this pull request; all PRs with a changelog label need to have release notes included as part of the PR. If you haven't added release notes already, please do. Release notes are added by creating a uniquely named file in the The basic format of the release notes (using markdown) should be: Thanks. |
|
@dipinhora i'm going to be testing this with the various "short-lived actors" tests soon-ish. |
|
Testing with the examples looks good. |
Prior to this commit, there could be race conditions with a actor
running concurrently on two different threads due to work being
done after an actors queue was marked empty.
This commit reworks the logic to ensure that the actors queue being
marked empty is the last thing that occurs in
ponyint_actor_runto prevent these race conditions.
This commit also adds logic in the cycle detector to not send any
messages to actors that are marked as pending destroy to ensure
that the cycle detector cannot create a race condition by sending
messages to a zombie actor (with
rc == 0) that is about to bereaped in the near future.