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

8.0: implement WaitForConfirmsAsync #999

Merged
merged 1 commit into from
Jan 11, 2021

Conversation

bollhals
Copy link
Contributor

@bollhals bollhals commented Jan 9, 2021

Proposed Changes

implements a async WaitForConfirms => does not block a thread anymore while waiting.

Types of Changes

  • Bug fix (non-breaking change which fixes issue Using blocking publisher confirms with concurrent publishers #959 )
  • 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

  • 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

This change partially fixes #959. Partially due to it only fixes the wait part and not the channel open part.
(Meaning as long as not all thread pool threads are blocked in opening channels, the publish works as expected. This can be achieved by either precreating channels or pooling the channels or making sure not all threads are started at the same time, so all create a channel at the same time (I'd argue this is already given by real world scenarios))

/// We need to close the socket, otherwise attempting to unload the domain
/// could cause a CannotUnloadAppDomainException
/// </remarks>
public void HandleDomainUnload(object sender, EventArgs ea)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

was unused

@@ -750,7 +741,7 @@ public void MaybeStartHeartbeatTimers()

public void StartMainLoop()
{
_mainLoopTask = Task.Run((Action)MainLoop);
_mainLoopTask = Task.Factory.StartNew(MainLoop, TaskCreationOptions.LongRunning);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This forces the creation of a new thread instead of blocking one of the threadpool

string s = "payload";
if (length > s.Length)
{
s.PadRight(length);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This didn't work as a string is immutable and the returned new string was unused. So it always sent "payload" for all length inputs. => I removed most of the different cases (when I changed it to send the actual size it ran into timeouts due to the huge amount of data it tried to send.)

@michaelklishin michaelklishin merged commit 17f5635 into rabbitmq:master Jan 11, 2021
@michaelklishin michaelklishin changed the title 7.0 - implement WaitForConfirmsAsync 8.0: implement WaitForConfirmsAsync Jan 11, 2021
lukebakken added a commit that referenced this pull request Oct 29, 2023
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.

Using blocking publisher confirms with concurrent publishers
2 participants