Skip to content

Acquire/Release Event Loop #725

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

Merged
merged 17 commits into from
May 12, 2025
Merged

Acquire/Release Event Loop #725

merged 17 commits into from
May 12, 2025

Conversation

xiazhvera
Copy link
Contributor

@xiazhvera xiazhvera commented Apr 24, 2025

Issue #, if available:

Description of changes:

  • What does this PR do
    This PR introduces APIs to acquire and release event loops, by acquiring and releasing the event loop group (ELG) that the event loop belongs to. If the event loop does not belong to any group, the acquire operation will fail. However, this case should never occur in practice, as users are expected to interact with the event loop group API directly.

  • Why do we need this
    Event loops are not ref-counted. For asynchronous APIs like the Dispatch Queue, it is necessary to retain a reference to the ELG ensures the event loop isn’t destroyed while async work is still pending. Besides, In cases where we need to process tasks on an event loop outside the context of a channel (e.g., request response client), we must ensure the event loop remains alive independently of the channel or channel bootstrap.
    These new APIs help to keep the event loop alive as needed.

Also merged with a previous similar PR: #536

Ideally socket should also get updated to acquire event loop (

struct aws_event_loop *event_loop;
), however, we frequently set/unset event loop in posix socket make things complicated (https://github.com/awslabs/aws-c-io/blob/get_eventloop_group/source/posix/socket.c#L449). I will leave the socket as it is for now.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@xiazhvera xiazhvera marked this pull request as draft April 24, 2025 22:04
@codecov-commenter
Copy link

codecov-commenter commented Apr 24, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.74%. Comparing base (7c00720) to head (b59954d).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #725      +/-   ##
==========================================
+ Coverage   78.60%   78.74%   +0.14%     
==========================================
  Files          30       30              
  Lines        6380     6385       +5     
==========================================
+ Hits         5015     5028      +13     
+ Misses       1365     1357       -8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@xiazhvera xiazhvera marked this pull request as ready for review April 24, 2025 22:58
Copy link
Contributor

@sbSteveK sbSteveK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional name change for clarity. Otherwise looks solid.

Copy link
Contributor

@bretambrose bretambrose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's talk about this

Copy link
Contributor

@waahm7 waahm7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar PR: #536. I don't know why that was not merged.

@xiazhvera xiazhvera dismissed bretambrose’s stale review May 9, 2025 18:56

There was concern about the original API was misleading—it suggested that it was acquiring an event loop directly, which is not accurate. In reality, the operation acquires the event loop group associated with the event loop, not the event loop itself. To avoid confusion and better reflect the actual behavior, we have updated the API name to clearly indicate that it acquires the event loop group through the event loop.

@xiazhvera xiazhvera merged commit 3d164ff into main May 12, 2025
51 checks passed
@xiazhvera xiazhvera deleted the get_eventloop_group branch May 12, 2025 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants