[network] handle onWriteReady socket failure #15137
Conversation
Signed-off-by: Asra Ali <[email protected]>
Signed-off-by: Asra Ali <[email protected]>
| ""); | ||
| Api::SysCallIntResult result = | ||
| socket_->getSocketOption(SOL_SOCKET, SO_ERROR, &error, &error_size); | ||
| RELEASE_ASSERT(result.rc_ == 0, |
There was a problem hiding this comment.
Should we change this RELEASE_ASSERT to ENVOY_BUG + error handling?
Also, are there other examples where getSocketOption is followed by an ASSERT or RELEASE_ASSERT?
There was a problem hiding this comment.
The other cases of getSocketOption all use error handling.
|
@sunjayBhatia I decided to change this to error handling as it was in release mode before the refactor when it was an |
asraa
changed the title
Signed-off-by: Asra Ali <[email protected]>
| @@ -656,8 +656,6 @@ void ConnectionImpl::onWriteReady() { | |||
| socklen_t error_size = sizeof(error); | |||
| Api::SysCallIntResult result = | |||
| socket_->getSocketOption(SOL_SOCKET, SO_ERROR, &error, &error_size); | |||
| RELEASE_ASSERT(result.rc_ == 0, | |||
| fmt::format("Failed to connect: {}", errorDetails(result.errno_))); | |||
|
|
|||
| if (error == 0) { | |||
There was a problem hiding this comment.
undefined behavior: error is not inialized if result.rc_ != 0
There was a problem hiding this comment.
I see, I was under the impression error would be set, but it's actually two separate things. I'll handle getSocketOption return error with an ENVOY_BUG (since this is unexpected) and close socket, and error != 0 additionally with close socket.
Checking my understanding. getSocketOption can fail for e.g. if the fd is invalid, or we don't have enough resources. If we succeed, the socket may still have a pending error returned with error that happened between socket calls. Maybe like connection refused?
| @@ -669,7 +667,8 @@ void ConnectionImpl::onWriteReady() { | |||
| return; | |||
| } | |||
| } else { | |||
| ENVOY_CONN_LOG(debug, "delayed connection error: {}", *this, error); | |||
| ENVOY_BUG(result.rc_ == 0, fmt::format("failed to connect: {}", errorDetails(result.errno_))); | |||
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
github-actions
bot
added
the
stale
asraa
removed
the
stale
|
Removing the stale, fell off my radar with some WIP tests. Will be revisiting next week. |
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
github-actions
bot
added
the
stale
|
This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
Signed-off-by: Asra Ali [email protected]
Commit Message: Remove
RELEASE_ASSERTand handle socket connection failureAdditional Description:
envoy/source/common/network/connection_impl.cc
Line 657 in 70fbf11
Risk Level: Arguably negative risk. Removes risk of crash in favor of error handling (that was already existing)