-
Notifications
You must be signed in to change notification settings - Fork 42
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
Handle System.InvalidOperationException in case of big body buffer send #220
Conversation
Signed-off-by: Gabriele Santomaggio <[email protected]>
Codecov ReportBase: 92.33% // Head: 92.33% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## main #220 +/- ##
=======================================
Coverage 92.33% 92.33%
=======================================
Files 94 94
Lines 8262 8262
Branches 651 651
=======================================
Hits 7629 7629
Misses 494 494
Partials 139 139 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. |
Signed-off-by: Gabriele Santomaggio <[email protected]>
Signed-off-by: Gabriele Santomaggio <[email protected]>
Signed-off-by: Gabriele Santomaggio <[email protected]>
Signed-off-by: Gabriele Santomaggio <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some early feedback, I understand the PR is still a WIP 🙂
Signed-off-by: Gabriele Santomaggio <[email protected]>
@@ -239,15 +239,11 @@ public static async Task<Client> Create(ClientParameters parameters, ILogger log | |||
|
|||
public async ValueTask<bool> Publish(Publish publishMsg) | |||
{ | |||
var publishTask = Publish<Publish>(publishMsg); | |||
if (!publishTask.IsCompletedSuccessfully) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For more info about this change, see:
https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca2012
} | ||
|
||
private void WriteCommand<T>(T command) where T : struct, ICommand | ||
private async Task WriteCommand<T>(T command) where T : struct, ICommand |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the main change.
It makes WriteCommand
async using WriteAsync
Add FlushAsync
instead of sync wait the task
{ | ||
_logger.LogWarning("Semaphore Wait timeout during publishing."); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this Async Await. In some case, it raises semaphorefullexception
after:
_logger.LogWarning("Semaphore Wait timeout during publishing.");
the code continue to work failing the scope of the lock
} | ||
|
||
return flushTask.Result.IsCompleted; | ||
await WriteCommand(command); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now everything is async with the await
we return true;
to leave the API compatibility
Signed-off-by: Gabriele Santomaggio <[email protected]>
var publishTask = | ||
_client.Publish(new SubEntryPublish(_publisherId, publishingId, | ||
CompressionHelper.Compress(subEntryMessages, compressionType))); | ||
if (!publishTask.IsCompletedSuccessfully) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I know, since this is a general-purpose library, every await
should have ConfigureAwait(false)
- https://devblogs.microsoft.com/dotnet/configureawait-faq/#when-should-i-use-configureawaitfalse
Signed-off-by: Gabriele Santomaggio <[email protected]>
Fixes #220 and part of #145
How to reproduce the issue:
See #221 (comment)
Important:
Main changes:
WriteCommand
Async:rabbitmq-stream-dotnet-client/RabbitMQ.Stream.Client/Connection.cs
Lines 99 to 114 in 5c695ef
Remove sync Waits (because of https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca2012) :
rabbitmq-stream-dotnet-client/RabbitMQ.Stream.Client/Connection.cs
Lines 89 to 97 in 5c695ef
Make the Async Blocking:
rabbitmq-stream-dotnet-client/RabbitMQ.Stream.Client/RawProducer.cs
Lines 161 to 164 in 5c695ef
It caused a lot of timeout operations but without any action to handle the problem.
Move the semaphore scope on the Producer Class:
rabbitmq-stream-dotnet-client/RabbitMQ.Stream.Client/Reliable/Producer.cs
Line 207 in 5c695ef
It caused
Client timeout Error
in case of multi-threads publishSigned-off-by: Gabriele Santomaggio [email protected]