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

Make connection events async #1677

Merged
merged 18 commits into from
Sep 18, 2024

Conversation

danielmarbach
Copy link
Collaborator

@danielmarbach danielmarbach commented Sep 17, 2024

Proposed Changes

Follow through on discussion in #1675 (comment)

@lukebakken mentioned his original intent was to make the events async across the board but stopped.

This PR would align the connection API surface

Types of Changes

What types of changes does your code introduce to this project?
Put an x in the boxes that apply

  • Bug fix (non-breaking change which fixes issue #NNNN)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause an observable behavior change in existing systems)
  • Documentation improvements (corrections, new content, etc)
  • Cosmetic change (whitespace, formatting, etc)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating
the PR. If you're unsure about any of them, don't hesitate to ask on the
mailing list. We're here to help! This is simply a reminder of what we are
going to look for before merging your code.

  • I have read the CONTRIBUTING.md document
  • I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
  • All tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in related repositories

Further Comments

If this is a relatively large or complex change, kick off the discussion by
explaining why you chose the solution you did and what alternatives you
considered, etc.

@lukebakken lukebakken self-requested a review September 17, 2024 15:55
@lukebakken lukebakken force-pushed the async-connection-events branch from e0ea90f to 830b1e4 Compare September 17, 2024 15:56
@lukebakken
Copy link
Contributor

Thank you @danielmarbach, I will review this ASAP. Just FYI, I merged #1675 and rebased your PR branch on main.

@lukebakken lukebakken self-assigned this Sep 17, 2024
@lukebakken lukebakken added this to the 7.0.0 milestone Sep 17, 2024
@danielmarbach
Copy link
Collaborator Author

Once this one is in I can change the other events too if you'd like

@lukebakken
Copy link
Contributor

Once this one is in I can change the other events too if you'd like

That would be great. From my attempt at doing this, I remember that it was the OnSessionShutdown event that, when made async, made everything go haywire in the test suite. That doesn't seem to be the case here (which is great).

@lukebakken
Copy link
Contributor

Is now the time to add CancellationToken to the AsyncEventHandler delegate?

@danielmarbach
Copy link
Collaborator Author

Is now the time to add CancellationToken to the AsyncEventHandler delegate?

My plan was to sweep through that after all the events are changed to be async as a dedicated PR and not mix things

@lukebakken lukebakken force-pushed the async-connection-events branch from 6eb842d to 8c7f8f9 Compare September 17, 2024 17:04
@danielmarbach
Copy link
Collaborator Author

Are you also adding the suffix to all the other events too?

@lukebakken
Copy link
Contributor

Are you also adding the suffix to all the other events too?

Everything that has changed in this PR. I'll do a commit per-rename.

_connectionBlockedAsyncWrapper.Takeover(other._connectionBlockedAsyncWrapper);
_connectionUnblockedAsyncWrapper.Takeover(other._connectionUnblockedAsyncWrapper);
_connectionShutdownAsyncWrapper.Takeover(other._connectionShutdownAsyncWrapper);
// TODO Why are other wrappers not taken over?
Copy link
Contributor

@lukebakken lukebakken Sep 17, 2024

Choose a reason for hiding this comment

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

There is only one wrapper that is missing from this list (_consumerAboutToBeRecoveredAsyncWrapper). The other wrappers are in the AutorecoveringConnection class, which remains the same instance when a connection is recovered. The reason TakeOver is necessary is that the inner Connection instance is a new one during connection recovery.

Copy link
Contributor

@lukebakken lukebakken left a comment

Choose a reason for hiding this comment

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

Renames are complete, I'll merge this once CI passes. Thanks!

@lukebakken lukebakken merged commit 6a45f13 into rabbitmq:main Sep 18, 2024
11 checks passed
@danielmarbach danielmarbach deleted the async-connection-events branch September 18, 2024 04:46
@danielmarbach
Copy link
Collaborator Author

Raised the PR for the session event #1679

@danielmarbach danielmarbach mentioned this pull request Sep 18, 2024
11 tasks
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.

2 participants