Skip to content
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

Fix bug where last few bytes on socket go unread #642

Merged
merged 18 commits into from
Jun 4, 2024
Merged

Fix bug where last few bytes on socket go unread #642

merged 18 commits into from
Jun 4, 2024

Conversation

graebm
Copy link
Contributor

@graebm graebm commented May 30, 2024

Issue:

aws-c-s3 is occasionally seeing errors when the server sends an HTTP response with a Connection: close header (meaning it intends to close the connection after the response is sent). The server is sending the full response, then immediately hanging up. But the last few bytes of the response never make it to the HTTP client.

Diagnosis:

If a peer closed their socket immediately after the last few bytes were sent, our socket code wasn't always reading those last few bytes.

On Linux, if a socket has unread data AND the peer has closed their side, the event from epoll has 2 flags set: EPOLLIN|EPOLLRDHUP. This means "the socket is has data to read AND the other side is closed (or half-closed) and won't be sending any more data".

Our socket handler code kinda did the right thing by "attempting" to read from the socket before shutting down the channel. But if the downstream read window reached 0, that "attempt" wouldn't read all the data.

Description of changes:

The socket handler no longer shuts down the channel in response to an error event. Instead, the error event queues more reads to happen. And only when read() reports that the socket is finished (due to error or EOF), will the socket handler shut down the channel.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov-commenter
Copy link

codecov-commenter commented May 30, 2024

Codecov Report

Attention: Patch coverage is 88.88889% with 2 lines in your changes missing coverage. Please review.

Project coverage is 80.05%. Comparing base (e762fd2) to head (56d92d3).

Files Patch % Lines
source/socket_channel_handler.c 81.81% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #642      +/-   ##
==========================================
- Coverage   80.11%   80.05%   -0.07%     
==========================================
  Files          28       28              
  Lines        5965     5967       +2     
==========================================
- Hits         4779     4777       -2     
- Misses       1186     1190       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -122,6 +122,10 @@ static void s_on_readable_notification(struct aws_socket *socket, int error_code
*/
static void s_do_read(struct socket_handler *socket_handler) {

if (socket_handler->shutdown_in_progress) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function didn't have any bugs. I'm just simplifying the code.

When this code was written, shutdown_in_progress could change half-way through the function, but since 2019 shutdown is always queued, so moving this check to the top of the function, instead of having it at 3 separate parts of a complicated flow

Comment on lines -223 to -225
if (error_code && !socket_handler->shutdown_in_progress) {
aws_channel_shutdown(socket_handler->slot->channel, error_code);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only line that truly HAD to change to fix this bug

Copy link
Contributor

@TingDaoK TingDaoK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

source/socket_channel_handler.c Show resolved Hide resolved
@graebm graebm merged commit 878b4fa into main Jun 4, 2024
34 of 35 checks passed
@graebm graebm deleted the read-to-eof branch June 4, 2024 16:27
graebm added a commit to awslabs/aws-crt-python that referenced this pull request Jun 5, 2024
**Issue:**

awscrt.s3.S3Client is occasionally seeing errors when the server sends an HTTP response with a `Connection: close` header (meaning it intends to close the connection after the response is sent). The server is sending the full response, then immediately hanging up. But the last few bytes of the response never make it to the HTTP client.

**Description of changes:**

Update submodules, bringing in this fix: awslabs/aws-c-io#642

```
aws-c-auth         v0.7.17 -> v0.7.22
aws-c-cal          v0.6.11 -> v0.6.15
aws-c-common       v0.9.15 -> v0.9.20
aws-c-http         v0.8.1 -> v0.8.2
aws-c-io           v0.14.7 -> v0.14.9
aws-c-mqtt         v0.10.3 -> v0.10.4
aws-c-s3           v0.5.7 -> v0.5.10
aws-c-sdkutils     v0.1.15 -> v0.1.16
s2n                v1.4.11 -> v1.4.15
```
graebm added a commit to awslabs/aws-crt-swift that referenced this pull request Jun 12, 2024
**Issue:**

We see occasional errors when the server sends an HTTP response with a `Connection: close` header (meaning it intends to close the connection after the response is sent). The server is sending the full response, then immediately hanging up. But the last few bytes of the response never make it to the HTTP client.

**Description of changes:**

Update submodules, bringing in this fix: awslabs/aws-c-io#642
```
aws-c-auth         v0.7.16 -> v0.7.22
aws-c-cal          v0.6.10 -> v0.6.15
aws-c-common       v0.9.14 -> v0.9.21
aws-c-http         v0.8.1 -> v0.8.2
aws-c-io           v0.14.6 -> v0.14.9
aws-c-sdkutils     v0.1.15 -> v0.1.16
s2n                v1.4.8 -> v1.4.16
```
graebm added a commit to awslabs/aws-crt-cpp that referenced this pull request Jun 12, 2024
**Issue:**

aws-c-s3 is occasionally seeing errors when the server sends an HTTP response with a `Connection: close` header (meaning it intends to close the connection after the response is sent). The server is sending the full response, then immediately hanging up. But the last few bytes of the response never make it to the HTTP client.

**Description of changes:**

Update submodules, bringing in this fix: awslabs/aws-c-io#642
```
aws-c-common       v0.9.19 -> v0.9.21
aws-c-http         v0.8.1 -> v0.8.2
aws-c-io           v0.14.8 -> v0.14.9
aws-c-s3           v0.5.9 -> v0.5.10
aws-lc             v1.28.0 -> v1.29.0
s2n                v1.4.15 -> v1.4.16
```
graebm added a commit to awslabs/aws-crt-java that referenced this pull request Jun 13, 2024
**Issue:**

We're occasionally seeing errors when the server sends an HTTP response with a `Connection: close` header (meaning it intends to close the connection after the response is sent). The server is sending the full response, then immediately hanging up. But the last few bytes of the response never make it to the HTTP client.

**Description of changes:**

Update submodules, bringing in this fix: awslabs/aws-c-io#642
```
aws-c-auth         v0.7.17 -> v0.7.22
aws-c-cal          v0.6.11 -> v0.6.15
aws-c-common       v0.9.15 -> v0.9.21
aws-c-http         v0.8.1 -> v0.8.2
aws-c-io           v0.14.7 -> v0.14.9
aws-c-mqtt         v0.10.3 -> v0.10.4
aws-c-sdkutils     v0.1.15 -> v0.1.16
s2n                v1.4.11 -> v1.4.16
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants