Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify unmuting of senders logic #3289

Closed
wants to merge 1 commit into from

Conversation

dipinhora
Copy link
Contributor

This commit removes an unecessary stack when unmuting actors that
may have been muted due to sending messages to an overload or
muted actor.

This commit removes an unecessary stack when unmuting actors that
may have been muted due to sending messages to an overload or
muted actor.
@dipinhora dipinhora added the do not merge This PR should not be merged at this time label Aug 29, 2019
@dipinhora
Copy link
Contributor Author

Marked as DO NOT MERGE as this needs thorough testing to ensure it is still deadlock free after the change. See: #3287 (comment)

@SeanTAllen
Copy link
Member

I think I remember why the stack exists:

commented in #3287 (comment)

      // This is safe because an actor can only ever be in a single scheduler's
      // mutemap

If we unmute before


    ponyint_mutemap_removeindex(&sched->mute_mapping, index);
    ponyint_muteref_free(mref);

then an actor could be rescheduled, send messages, get muted again and end up in more than 1 mute map.

** i think **

@SeanTAllen
Copy link
Member

Yeah, the stack separation is needed Dipin. Per the reason above. This should be closed. A comment stating why the stack exists, should be added to be more clear than my not at all clear comment that was originally put in.

@dipinhora
Copy link
Contributor Author

@SeanTAllen i'm trying to follow the code and i'm not seeing the issue you mention about an actor ending up in more than one mutemap. it is very likely i'm missing something obvious.

the following is what i'm able to understand (in relation to the modified code in this PR):

  1. ponyint_sched_unmute_senders unmutes senders that were muted for sending messages to the actor
  2. ponyint_sched_unmute_senders gets called by a scheduler receiving a SCHED_UNMUTE_ACTOR message for a specific actor
  3. SCHED_UNMUTE_ACTOR messages are sent via the ponyint_sched_start_global_unmute function
  4. ponyint_sched_start_global_unmute is called by ponyint_sched_unmute_senders, ponyint_actor_unsetoverloaded, or pony_release_backpressure
    1. In the cases of ponyint_actor_unsetoverloaded and pony_release_backpressure, the actor was not muted (because it was either overloaded or under pressure)
    2. In the case of ponyint_sched_unmute_senders, the actor (sender) was muted due to sending to either another muted actor or a overloaded or under pressure actor and it has been unmuted a few lines earlier by the time ponyint_sched_start_global_unmute is called for the muted actor
    3. In all cases, by the time ponyint_sched_start_global_unmute runs, the actor is guaranteed to be unmuted
  5. Given the above, when a muted actor gets unmuted in ponyint_sched_unmute_senders, it can send messages to the original actor it sent to and was muted because of, without getting muted again for that send. It might get muted again for sending to a different actor (and end up in another mutemap) but that is possible both before and after the changes in this PR.
  6. The note about This is safe because an actor can only ever be in a single scheduler's mutemap is not relevant because the code order guarantees that prior to ponyint_sched_start_global_unmute being called, all atomic operations on muted that modify its muted count or its is_muted status have been completed so it doesn't break the guarantees needed/expected by that comment which are about isolation for atomic operations being safe to concurrent writes.

it is very possible that i'm misunderstanding or missing something. it is also possible, that there is some other issue that i'm not understanding that having the stack resolves. either way, i would appreciate it if you could correct any misunderstandings or gaps in my understanding.

my plan is that once i understand the nuances that i can properly document why the stack is needed or if it is not needed, we can remove it and document whatever else might need to be documented in terms of deadlocks and race conditions in relation to the backpressure/muting logic.

@SeanTAllen
Copy link
Member

@dipinhora I'll review your comments when I get back from vacation later in the month.

@dipinhora
Copy link
Contributor Author

@SeanTAllen okey dokey. have fun vacating.

@SeanTAllen
Copy link
Member

@dipinhora i clearly dropped the ball on this and then forgot the ball even existed. I'll get on that sometime, hopefully soon-ish.

Base automatically changed from master to main February 8, 2021 23:02
@SeanTAllen
Copy link
Member

@Theodus as you have been doing lots of work in the area of backpressure, can you give this a review and decide if we should move forward or not?

@SeanTAllen SeanTAllen requested a review from Theodus February 22, 2021 13:50
@dipinhora
Copy link
Contributor Author

Superseded by #4151

@dipinhora dipinhora closed this Jun 22, 2022
@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Jun 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss during sync Should be discussed during an upcoming sync do not merge This PR should not be merged at this time
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants