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.

Sign up for GitHub

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

Jump to bottom

Investigate how warp handles HTTP/2 GOAWAY #673

Closed
nh2 opened this issue Feb 14, 2018 · 4 comments · Fixed by #759
Closed

Investigate how warp handles HTTP/2 GOAWAY #673

nh2 opened this issue Feb 14, 2018 · 4 comments · Fixed by #759
Assignees
@kazu-yamamoto
Labels
http2

Comments

@nh2
Copy link

nh2 commented Feb 14, 2018

Hi @kazu-yamamoto,

yesterday I found a problem in nginx's implementation of the HTTP/2 GOAWAY feature. It is very easy to get that feature wrong. I have written up and linked the details on https://trac.nginx.org/nginx/ticket/1250#comment:4.

Would you be able to tell me if warp handles this correctly currently, or if that's not clear yet, how it implements sending GOAWAY or where the bit that handles this is in the code?

Thanks!
@kazu-yamamoto
Copy link
Contributor

This may be relating to yesodweb/yesod#1479

@kazu-yamamoto
Copy link
Contributor

Warp does not make use of shutdown at this moment. So, the same thing would happen in many cases including sending GOAWAY.

@nh2 nh2 mentioned this issue Feb 23, 2018
Open
@nh2
Copy link
Author

nh2 commented Feb 23, 2018

@kazu-yamamoto Yes, it's very likely the issue you see in the other bug.

@kazu-yamamoto
Copy link
Contributor

I think I will be able to take time to consider this in March.

@kazu-yamamoto kazu-yamamoto self-assigned this Apr 9, 2018
@kazu-yamamoto kazu-yamamoto mentioned this issue Jun 28, 2019
Merged
kazu-yamamoto added a commit to kazu-yamamoto/wai that referenced this issue Jul 1, 2019
@kazu-yamamoto
implementing graceful closing of sockets.
6d9d1a1 
Sometime HTTP/1 "Connection: close" and HTTP/2 GOAWAY are not
delivered to clients sometime because close(2) is called
immediately. This is reported in
yesodweb#673

We can reproduce this with h2spec:

% h2spec http2/5.4 http2/7
  5. Streams and Multiplexing
    5.4. Error Handling
      5.4.1. Connection Error Handling
        × 1: Sends an invalid PING frame for connection close
          -> The endpoint MUST close the TCP connection
             Expected: Connection closed
               Actual: Error: read tcp 127.0.0.1:56529->127.0.0.1:80: read: connection reset by peer

  7. Error Codes
    × 1: Sends a GOAWAY frame with unknown error code
      -> The endpoint MUST NOT trigger any special behavior.
         Expected: Connection closed
                   PING Frame (length:8, flags:0x01, stream_id:0, opaque_data:h2spec)
           Actual: Error: read tcp 127.0.0.1:56578->127.0.0.1:80: read: connection reset by peer

For grace closing, we do:
- Send HTTP level bye
- Send TCP FIN by shutdown (2)
- Recv TCP FIN by recv(2)
- Finally release the socket resource by close(2).
kazu-yamamoto added a commit to kazu-yamamoto/wai that referenced this issue Jul 10, 2019
@kazu-yamamoto
HTTP/2 GOAWAY are not delivered to clients sometime because close(2)
07e074e 
is called immediately. This is reported in yesodweb#673.

We can reproduce this with h2spec:

% h2spec http2/5.4 http2/7
  5. Streams and Multiplexing
    5.4. Error Handling
      5.4.1. Connection Error Handling
        × 1: Sends an invalid PING frame for connection close
          -> The endpoint MUST close the TCP connection
             Expected: Connection closed
               Actual: Error: read tcp 127.0.0.1:56529->127.0.0.1:80: read: connection reset by peer

  7. Error Codes
    × 1: Sends a GOAWAY frame with unknown error code
      -> The endpoint MUST NOT trigger any special behavior.
         Expected: Connection closed
                   PING Frame (length:8, flags:0x01, stream_id:0, opaque_data:h2spec)
           Actual: Error: read tcp 127.0.0.1:56578->127.0.0.1:80: read: connection reset by peer

To fix this, we use "gracefulClose" in the network library.
kazu-yamamoto added a commit to kazu-yamamoto/wai that referenced this issue Aug 26, 2019
@kazu-yamamoto
HTTP/2 GOAWAY are not delivered to clients sometime because close(2)
70364b4 
is called immediately. This is reported in yesodweb#673.

We can reproduce this with h2spec:

% h2spec http2/5.4 http2/7
  5. Streams and Multiplexing
    5.4. Error Handling
      5.4.1. Connection Error Handling
        × 1: Sends an invalid PING frame for connection close
          -> The endpoint MUST close the TCP connection
             Expected: Connection closed
               Actual: Error: read tcp 127.0.0.1:56529->127.0.0.1:80: read: connection reset by peer

  7. Error Codes
    × 1: Sends a GOAWAY frame with unknown error code
      -> The endpoint MUST NOT trigger any special behavior.
         Expected: Connection closed
                   PING Frame (length:8, flags:0x01, stream_id:0, opaque_data:h2spec)
           Actual: Error: read tcp 127.0.0.1:56578->127.0.0.1:80: read: connection reset by peer

To fix this, we use "gracefulClose" in the network library.
kazu-yamamoto added a commit to kazu-yamamoto/wai that referenced this issue Sep 10, 2019
@kazu-yamamoto
HTTP/2 GOAWAY are not delivered to clients sometime because close(2)
aee7377 
is called immediately. This is reported in yesodweb#673.

We can reproduce this with h2spec:

% h2spec http2/5.4 http2/7
  5. Streams and Multiplexing
    5.4. Error Handling
      5.4.1. Connection Error Handling
        × 1: Sends an invalid PING frame for connection close
          -> The endpoint MUST close the TCP connection
             Expected: Connection closed
               Actual: Error: read tcp 127.0.0.1:56529->127.0.0.1:80: read: connection reset by peer

  7. Error Codes
    × 1: Sends a GOAWAY frame with unknown error code
      -> The endpoint MUST NOT trigger any special behavior.
         Expected: Connection closed
                   PING Frame (length:8, flags:0x01, stream_id:0, opaque_data:h2spec)
           Actual: Error: read tcp 127.0.0.1:56578->127.0.0.1:80: read: connection reset by peer

To fix this, we use "gracefulClose" in the network library.
@hasufell hasufell mentioned this issue Dec 6, 2019
Closed
@nh2 nh2 mentioned this issue May 20, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kazu-yamamoto kazu-yamamoto

Labels
http2
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

shutdown instead of close. kazu-yamamoto/wai
2 participants
@kazu-yamamoto @nh2