Fix pending requests failed with ResultConnectError when disconnecting #322
Conversation
This file contains hidden or 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
BewareMyPower
force-pushed
the
bewaremypower/fix-close-error
branch
2 times, most recently
from
559fce9 to
74bb9a0
Compare
### Motivation When there are multiple pending requests in the same `ClientConnection`, if one request failed with a retryable error, e.g. the `ServiceUnitNotReady` error when finding the owner broker of a topic, the socket will be closed in `checkServerError` and `close()` will be called subsequently in `handleRead` (`err` is `eof` or `operation_failed`). However, the default value of 1st parameter is `ResultConnectError` for `close`, which is not retryable. ### Modifications If the connection is disconnected by the client, pass `ResultDisconnected` to `close` and treat it as retryable. closeSocket is replaced with close(ResultDisconnected) to avoid the connection being the status that socket is closed but TLS stream is not closed.
BewareMyPower
force-pushed
the
bewaremypower/fix-close-error
branch
from
74bb9a0 to
8c99505
Compare
BewareMyPower
added a commit
to streamnative/pulsar-client-cpp
that referenced
this pull request
Oct 5, 2023
apache#322) ### Motivation When there are multiple pending requests in the same `ClientConnection`, if one request failed with a retryable error, e.g. the `ServiceUnitNotReady` error when finding the owner broker of a topic, the socket will be closed in `checkServerError` and `close()` will be called subsequently in `handleRead` (`err` is `eof` or `operation_failed`). However, the default value of 1st parameter is `ResultConnectError` for `close`, which is not retryable. ### Modifications If the connection is disconnected by the client, pass `ResultDisconnected` to `close` and treat it as retryable. closeSocket is replaced with close(ResultDisconnected) to avoid the connection being the status that socket is closed but TLS stream is not closed.
merlimat
approved these changes
Oct 6, 2023
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.
Motivation
When there are multiple pending requests in the same
ClientConnection, if one request failed with a retryable error, e.g. theServiceUnitNotReadyerror when finding the owner broker of a topic, the socket will be closed incheckServerErrorandclose()will be called subsequently inhandleRead(erriseoforoperation_failed). However, the default value of 1st parameter isResultConnectErrorforclose, which is not retryable.Modifications
If the connection is disconnected by the client, pass
ResultDisconnectedtocloseand treat it as retryable.closeSocketis replaced withclose(ResultDisconnected)to avoid the connection being the status that socket is closed but TLS stream is not closed.