Skip to content

Improve the reconnect #230

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 5 commits into from
Feb 6, 2023
Merged

Improve the reconnect #230

merged 5 commits into from
Feb 6, 2023

Conversation

Gsantomaggio
Copy link
Member

Handle some edge cases where the producer or consumer could go into a deadlock waiting for the semaphore.

Add the Cancellation token producer side and call the token cancellation when the producer and the consumer are forced to disconnect.

I did this CaosTest for the stream client.

Where the connections are closed randomly.

Messages sent: 344800 -Messages confirmed: 227737 - Messages error: 1024 - Total 228761  Messages consumed: 169337
Messages sent: 397000 -Messages confirmed: 279919 - Messages error: 116082 - Total 396001  Messages consumed: 206422

Client should be able to reconnect and continue to produce and consume

Signed-off-by: Gabriele Santomaggio G.santomaggio@gmail.com

Handle some edge cases where the producer or consumer could go into a deadlock waiting for the semaphore.

Add the Cancellation token producer side and call the token cancellation when the producer and the consumer
are forced to disconnect.

Signed-off-by: Gabriele Santomaggio <G.santomaggio@gmail.com>
@Gsantomaggio
Copy link
Member Author

cc @iuribrindeiro @ricardSiliuk @ricsiLT

@codecov
Copy link

codecov bot commented Feb 3, 2023

Codecov Report

Base: 92.30% // Head: 92.86% // Increases project coverage by +0.56% 🎉

Coverage data is based on head (de0ad58) compared to base (ea72747).
Patch coverage: 99.18% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #230      +/-   ##
==========================================
+ Coverage   92.30%   92.86%   +0.56%     
==========================================
  Files          95       96       +1     
  Lines        8343     8383      +40     
  Branches      653      654       +1     
==========================================
+ Hits         7701     7785      +84     
+ Misses        506      467      -39     
+ Partials      136      131       -5     
Impacted Files Coverage Δ
RabbitMQ.Stream.Client/RawProducer.cs 89.62% <94.44%> (+2.70%) ⬆️
RabbitMQ.Stream.Client/StreamSystem.cs 82.59% <96.87%> (-0.14%) ⬇️
RabbitMQ.Stream.Client/AbstractEntity.cs 100.00% <100.00%> (ø)
RabbitMQ.Stream.Client/Client.cs 93.78% <100.00%> (+2.01%) ⬆️
RabbitMQ.Stream.Client/Compression.cs 100.00% <100.00%> (+1.02%) ⬆️
RabbitMQ.Stream.Client/Connection.cs 92.10% <100.00%> (+0.06%) ⬆️
RabbitMQ.Stream.Client/HeartBeatHandler.cs 100.00% <100.00%> (ø)
RabbitMQ.Stream.Client/MetaData.cs 90.00% <100.00%> (ø)
RabbitMQ.Stream.Client/RasSuperStreamConsumer.cs 94.19% <100.00%> (ø)
RabbitMQ.Stream.Client/RawConsumer.cs 85.71% <100.00%> (+1.40%) ⬆️
... and 19 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

}

// Safe close
// the client can be closed only if the publishers are == 0
// not a public method used internally by producers and consumers
internal CloseResponse MaybeClose(string reason)
internal async Task<CloseResponse> MaybeClose(string reason)

Choose a reason for hiding this comment

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

{
if (publishers.Count == 0 && consumers.Count == 0)
{
return Close(reason).Result;
await Close(reason).ConfigureAwait(false);
Copy link

@iuribrindeiro iuribrindeiro Feb 3, 2023

Choose a reason for hiding this comment

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

❤️

@iuribrindeiro
Copy link

cc @iuribrindeiro @ricardSiliuk @ricsiLT

@Gsantomaggio please see: #231

@ricardSiliuk
Copy link
Contributor

Should this token stay private/protected or be made public? Just thinking "out loud" but I'd probably want to know if consumer/producer was somehow unexpectedly cancelled by linking new CTS with your Token?

Though if the only situation where this happens is when end user calls Close(), maybe he doesn't need to know about your CTS... not sure.

iuribrindeiro and others added 4 commits February 6, 2023 10:49
…t and create producer (#231)

* ConfigureAwait(false) for close stream, consumer, producer connections, query offset and create producer

* Apply roslynator.exe fixes

* Do not warn about ConfigureAwait in tests, fix more instances

* Fixing ConfigureAwait(false) for the rest of the lib

---------

Co-authored-by: Luke Bakken <luke@bakken.io>
Improve super stream example

Signed-off-by: Gabriele Santomaggio <G.santomaggio@gmail.com>
Signed-off-by: Gabriele Santomaggio <G.santomaggio@gmail.com>
Signed-off-by: Gabriele Santomaggio <G.santomaggio@gmail.com>
@Gsantomaggio Gsantomaggio merged commit 05a6716 into main Feb 6, 2023
@Gsantomaggio Gsantomaggio deleted the improvement_reconnect branch February 6, 2023 13:38
@Gsantomaggio
Copy link
Member Author

Thank you @iuribrindeiro @lukebakken @ricardSiliuk

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.

3 participants