fix: SendAsync break FlushAsync when RentConnection throw exception#1762
fix: SendAsync break FlushAsync when RentConnection throw exception#1762witskeeper wants to merge 1 commit intodotnetcore:masterfrom
Conversation
When `_connectionPool.RentConnection() ` throw exception The `publisher.PublishAsync` will throw exception,
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the exception handling in the SendAsync method by introducing nested try-catch blocks. The connection acquisition is moved inside the outer try block, and a new outer catch handler is added to handle exceptions during connection rental.
Key changes:
- Connection rental moved from outside try-catch into outer try block
- Added nested try-catch-finally structure with inner block handling message sending and outer block handling connection rental failures
- Outer catch block uses different exception variable name (
evsex)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try | ||
| { | ||
| var msg = new Msg(message.GetName(), message.Body.ToArray()); | ||
| foreach (var header in message.Headers) | ||
| var connection = _connectionPool.RentConnection(); | ||
| try | ||
| { |
There was a problem hiding this comment.
The outer try-catch structure introduces a bug. If RentConnection() at line 35 throws an exception, the connection variable will not be initialized, but the finally block at line 67 will still attempt to call _connectionPool.Return(connection) with an uninitialized variable. Consider removing the outer try-catch and handling RentConnection() exceptions separately, or ensure the finally block checks if the connection was successfully initialized before returning it.
| } | ||
| catch (Exception ex) | ||
| { | ||
| var warpEx = new PublisherSentFailedException(ex.Message, ex); |
There was a problem hiding this comment.
Variable name 'warpEx' appears to be a misspelling. Consider renaming to 'wrappedEx' or 'wrapEx' for clarity.
| { | ||
| var warpEx = new PublisherSentFailedException(ex.Message, ex); | ||
|
|
||
| var warpEx = new PublisherSentFailedException(e.Message, e); |
There was a problem hiding this comment.
Variable name 'warpEx' appears to be a misspelling. Consider renaming to 'wrappedEx' or 'wrapEx' for clarity.
| catch (Exception e) | ||
| { | ||
| var warpEx = new PublisherSentFailedException(ex.Message, ex); | ||
|
|
||
| var warpEx = new PublisherSentFailedException(e.Message, e); | ||
| return OperateResult.Failed(warpEx); | ||
| } |
There was a problem hiding this comment.
Generic catch clause.
| catch (Exception ex) | ||
| { | ||
| var warpEx = new PublisherSentFailedException(ex.Message, ex); | ||
|
|
||
| throw new PublisherSentFailedException("NATS message send failed, no consumer reply!"); | ||
| return OperateResult.Failed(warpEx); | ||
| } |
There was a problem hiding this comment.
Generic catch clause.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| catch (Exception e) | ||
| { | ||
| var warpEx = new PublisherSentFailedException(ex.Message, ex); | ||
|
|
||
| var warpEx = new PublisherSentFailedException(e.Message, e); | ||
| return OperateResult.Failed(warpEx); | ||
| } |
There was a problem hiding this comment.
The outer catch block at lines 70-74 will never catch exceptions from RentConnection() because the connection variable used in the finally block at line 67 would be uninitialized if RentConnection() throws. This creates a potential compilation error or null reference issue. Consider initializing connection to null before the try block and adding a null check in the finally block, similar to the RabbitMQ implementation pattern.
| } | ||
| catch (Exception ex) | ||
| { | ||
| var warpEx = new PublisherSentFailedException(ex.Message, ex); |
There was a problem hiding this comment.
Corrected spelling of 'warp' to 'wrap' in variable name 'warpEx' to 'wrapEx'.
| { | ||
| var warpEx = new PublisherSentFailedException(ex.Message, ex); | ||
|
|
||
| var warpEx = new PublisherSentFailedException(e.Message, e); |
There was a problem hiding this comment.
Corrected spelling of 'warp' to 'wrap' in variable name 'warpEx' to 'wrapEx'.
1 participant
Description:
When
_connectionPool.RentConnection()throw exceptionThe
publisher.PublishAsyncwill throw exception.Issue(s) addressed:
Changes:
Affected components:
How to test:
Provide a step-by-step guide on how to test your changes. Include any relevant information like environment setup, dependencies, etc.
Additional notes (optional):
Provide any additional notes or context that may be relevant to the changes.
Checklist:
Reviewers: