Refactor client to return error on receiver.Receive #707
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Oleg Kulyk <oleg.kulyk@checkmarx.com>
n3wscott
reviewed
Aug 16, 2021
| if err != nil { | ||
| cecontext.LoggerFrom(ctx).Warn("Error while receiving a message: ", err) | ||
| continue | ||
| errChan <- err |
There was a problem hiding this comment.
not all errors cause all protocols to not respond, some would like the message to be returned via Invoke, this change does not seem like it will work for all protocols.
Is there a change to the amqp impl that would allow it to handle the error? Like, perhaps it needs to respond with a io.EOF when the the server is disconnected.
There was a problem hiding this comment.
Thanks for the review.
I think it does make send to send io.EOF in case connection is down, I'll look in that direction.
|
closing in favor of #750 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Oleg Kulyk oleg.kulyk@checkmarx.com
Fixes #685 and #684
Current behavior locks down consumer in an infinite loop if connection to Rabbitmq is closed. When analyzing the client code I saw no real purpose in "hiding" error coming from Receiver and just continuing running.
Current change should not introduce any b/c breaking issues, unless the client code depends on consumer not "dying" if connection is lost (for which I struggle to come up with a meaningful example).
The fix for test came after inspecting locally what would be the outcome if err is retuned (in which case test panicked because of "fail" in running goroutine after the
t.Runprocess is finished).