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

Disable incorrect runtime assert for ASIO thread shutdown. #4122

Merged
merged 1 commit into from
May 21, 2022

Conversation

jemc
Copy link
Member

@jemc jemc commented May 20, 2022

When shutting down the ASIO thread and destroying its message queue,
it is possible that there may be some message(s) in flight to it,
meaning that even though we drained the queue before destroying,
the queue may be non-empty by the time we get to destroying it.
Hence, we cannot reliably assert that the queue is non-empty.

However, for actors, we can proved definitively that an actor
is done receiving messages before we destroy its message queue,
so we want to retain the runtime assert for the actor case.

This change updates the assert to be conditional based on
a boolean parameter to the ponyint_messageq_destroy function.

For more context, see this Zulip thread.

@jemc jemc requested a review from SeanTAllen May 20, 2022 23:10
@jemc jemc self-assigned this May 20, 2022
@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label May 20, 2022
@jemc jemc added the changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge label May 21, 2022
@ponylang-main
Copy link
Contributor

Hi @jemc,

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 .release-notes directory. We suggest you call the file 4122.md to match the number of this pull request.

The basic format of the release notes (using markdown) should be:

## Title

End user description of changes, why it's important,
problems it solves etc.

If a breaking change, make sure to include 1 or more
examples what code would look like prior to this change
and how to update it to work after this change.

Thanks.

src/libponyrt/actor/messageq.c Outdated Show resolved Hide resolved
@SeanTAllen SeanTAllen added the do not merge This PR should not be merged at this time label May 21, 2022
@SeanTAllen
Copy link
Member

This looks good to merge once release notes are added.

When shutting down the ASIO thread and destroying its message queue,
it is possible that there may be some message(s) in flight to it,
meaning that even though we drained the queue before destroying,
the queue may be non-empty by the time we get to destroying it.
Hence, we cannot reliably assert that the queue is non-empty.

However, for actors, we can proved definitively that an actor
is done receiving messages before we destroy its message queue,
so we want to retain the runtime assert for the actor case.

This change updates the assert to be conditional based on
a boolean parameter to the `ponyint_messageq_destroy` function.

For more context, see [this Zulip thread](https://ponylang.zulipchat.com/#narrow/stream/190365-runtime/topic/ponyint_messageq_destroy.20assert.20fail).
@jemc jemc force-pushed the fix/conditional-messageq-destroy-assert branch from a26988b to d770750 Compare May 21, 2022 17:32
@jemc jemc merged commit 5756740 into ponylang:main May 21, 2022
@ponylang-main ponylang-main removed the discuss during sync Should be discussed during an upcoming sync label May 21, 2022
github-actions bot pushed a commit that referenced this pull request May 21, 2022
github-actions bot pushed a commit that referenced this pull request May 21, 2022
@jemc jemc deleted the fix/conditional-messageq-destroy-assert branch May 21, 2022 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge 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