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

Improve logic around cancelling consumers and closing models. #120

Merged
merged 7 commits into from
Dec 11, 2022

Conversation

Quogu
Copy link

@Quogu Quogu commented Dec 6, 2022

There are a couple of fixes in here, which are separated out into separate commits, specifically:

  • Forbidding duplicate consumer tags.
  • Cancelling consumers when a model is closed / disposed.
  • Making sure received events wouldn't fire after consumers were cancelled.
  • Cancel delivery of any more messages once the model is closed, to more closely mirror the RabbitMQ client.
  • Fixing a nasty deadlock issue when calling Close from inside a Received callback. This is not perfect, as if you manage to switch execution contexts inside the Receive handler it can still throw, but that's very difficult to handle. I was attempting to remove this Wait but attempting to do so unearths a limitation of the FakeConnectionFactory that breaks the ExchangeHeadersTests and ExchangeFanoutTests because they use two connections. This is something I'd like to look into more in the future but I don't have time right now.

Apologies for the several disparate changes in a single MR, I kept going until my code under test passed tests following the upgrade to 2.3.0. Hopefully the fact that it's in different commits allows for easy review, but if you'd like me to split it into separate MRs, please do let me know (and thanks for all your time reviewing my MRs recently, @odalet !)

@odalet
Copy link

odalet commented Dec 8, 2022

Hi! No need to apologize: if you've had a look at the Git tree, you probably understood I'm not an Ayatollah of Git cleanlyness! As long as the changes get pushed to master, I'm ok with that!

However, I chose to merge PR #121 before yours because:

So, I'll have to trouble you to adapt your PR so that it matches the latest developments!

@Quogu
Copy link
Author

Quogu commented Dec 9, 2022

That makes sense to me, #121 looks great, apologies to Patrik for making his tests so much slower!

I've rebased my work on top of the new head of master and made a few small improvements, so it should be good to go again now. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants