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

improve: handle COMPRESSION_ERROR to reduce the error log displayed #745

Merged
merged 1 commit into from
Mar 2, 2025
Merged

improve: handle COMPRESSION_ERROR to reduce the error log displayed #745

merged 1 commit into from
Mar 2, 2025

Conversation

yuweizzz
Copy link
Contributor

the reason why always has COMPRESSION_ERROR happened: the hpack context lost.

two example:

  1. single tcp connection, multi stream on. Before this pr, the hpack DynamicTable is set to 0, so the next headers frame could not be decoded.

run:

nghttp  https://localhost:4444/1234d -m 3

output:

2025/02/28 11:52:33 [http2 request] Dump HTTP2 Frame error: connection error: COMPRESSION_ERROR
2025-02-28T11:52:33+08:00 ??? UUID:210635_210635_nghttp_5_1_[::1]:54222-[::1]:4444, Name:HTTP2Request, Type:2, Length:618

Frame Type	=>	SETTINGS
Frame StreamID	=>	0

Frame Type	=>	SETTINGS
Frame StreamID	=>	0

Frame Type	=>	PRIORITY
Frame StreamID	=>	3

Frame Type	=>	PRIORITY
Frame StreamID	=>	5

Frame Type	=>	PRIORITY
Frame StreamID	=>	7

Frame Type	=>	PRIORITY
Frame StreamID	=>	9

Frame Type	=>	PRIORITY
Frame StreamID	=>	11

Frame Type	=>	HEADERS
Frame StreamID	=>	13
header field ":method" = "GET"
header field ":path" = "/1234d"
header field ":scheme" = "https"
header field ":authority" = "localhost:4444"
header field "accept" = "*/*"
header field "accept-encoding" = "gzip, deflate"
header field "user-agent" = "nghttp2/1.52.0"

this could be fixed in this pr, those frame use same hpack decoder.

  1. single tcp connection, single stream on, and than start a new stream in same tcp connection. first incoming event will work fine, but the new stream will get COMPRESSION_ERROR, because they use different event worker and never use same hpack decoder.
2025-02-28T12:00:15+08:00 ??? UUID:155734_155734_nginx_4_1_10.0.2.2:60482-10.0.2.5:4444, Name:HTTP2Response, Type:4, Length:992

Frame Type	=>	SETTINGS
Frame StreamID	=>	0

Frame Type	=>	WINDOW_UPDATE
Frame StreamID	=>	0

Frame Type	=>	SETTINGS
Frame StreamID	=>	0

Frame Type	=>	HEADERS
Frame StreamID	=>	1
header field ":status" = "404"
header field "server" = "openresty/1.25.3.2"
header field "date" = "Fri, 28 Feb 2025 04:00:14 GMT"
header field "content-type" = "text/html"
header field "content-length" = "561"

Frame Type	=>	DATA
Frame StreamID	=>	1
<html>
<head><title>404 Not Found</title></head>
<body>
<center><h1>404 Not Found</h1></center>
<hr><center>openresty/1.25.3.2</center>
</body>
</html>
<!-- a padding to disable MSIE and Chrome friendly error page -->
<!-- a padding to disable MSIE and Chrome friendly error page -->
<!-- a padding to disable MSIE and Chrome friendly error page -->
<!-- a padding to disable MSIE and Chrome friendly error page -->
<!-- a padding to disable MSIE and Chrome friendly error page -->
<!-- a padding to disable MSIE and Chrome friendly error page -->


2025-02-28T12:00:15+08:00 ??? UUID:155734_155734_nginx_4_0_10.0.2.2:60482-10.0.2.5:4444, Name:HTTP2Request, Type:2, Length:1172

Frame Type	=>	SETTINGS
Frame StreamID	=>	0

Frame Type	=>	WINDOW_UPDATE
Frame StreamID	=>	0

Frame Type	=>	HEADERS
Frame StreamID	=>	1
header field ":method" = "GET"
header field ":authority" = "localhost:4444"
header field ":scheme" = "https"
header field ":path" = "/123"
header field "sec-ch-ua" = "\"Not(A:Brand\";v=\"99\", \"Google Chrome\";v=\"133\", \"Chromium\";v=\"133\""
header field "sec-ch-ua-mobile" = "?0"
header field "sec-ch-ua-platform" = "\"Windows\""
header field "upgrade-insecure-requests" = "1"
header field "user-agent" = "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/133.0.0.0 Safari/537.36"
header field "accept" = "text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3;q=0.7"
header field "sec-fetch-site" = "none"
header field "sec-fetch-mode" = "navigate"
header field "sec-fetch-user" = "?1"
header field "sec-fetch-dest" = "document"
header field "accept-encoding" = "gzip, deflate, br, zstd"
header field "accept-language" = "zh-CN,zh;q=0.9"
header field "priority" = "u=0, i"

Frame Type	=>	SETTINGS
Frame StreamID	=>	0

2025-02-28T12:00:16+08:00 ??? UUID:155734_155734_nginx_5_1_10.0.2.2:60483-10.0.2.5:4444, Name:HTTP2Response, Type:4, Length:93

Frame Type	=>	SETTINGS
Frame StreamID	=>	0

Frame Type	=>	WINDOW_UPDATE
Frame StreamID	=>	0

2025/02/28 12:00:24 [http2 response] Dump HTTP2 Frame error: connection error: COMPRESSION_ERROR
2025-02-28T12:00:24+08:00 ??? UUID:155734_155734_nginx_4_1_10.0.2.2:60482-10.0.2.5:4444, Name:HTTP2Response, Type:4, Length:855

Frame Type	=>	HEADERS
Frame StreamID	=>	3
header field ":status" = "404"
header field "server" = "openresty/1.25.3.2"
header field "date" = "Fri, 28 Feb 2025 04:00:23 GMT"
header field "content-type" = "text/html"
header field "content-length" = "561"

Frame Type	=>	DATA
Frame StreamID	=>	3
<html>
<head><title>404 Not Found</title></head>
<body>
<center><h1>404 Not Found</h1></center>
<hr><center>openresty/1.25.3.2</center>
</body>
</html>
<!-- a padding to disable MSIE and Chrome friendly error page -->
<!-- a padding to disable MSIE and Chrome friendly error page -->
<!-- a padding to disable MSIE and Chrome friendly error page -->
<!-- a padding to disable MSIE and Chrome friendly error page -->
<!-- a padding to disable MSIE and Chrome friendly error page -->
<!-- a padding to disable MSIE and Chrome friendly error page -->

this problem is quite hard to fix now.

@yuweizzz yuweizzz mentioned this pull request Feb 28, 2025
Closed
@dosubot dosubot bot added the improve label Feb 28, 2025
cfc4n
cfc4n approved these changes Mar 2, 2025
View reviewed changes
Copy link
Member

@cfc4n cfc4n left a comment

Choose a reason for hiding this comment

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

LGTM

@cfc4n cfc4n merged commit 12c144d into gojue:master Mar 2, 2025
7 checks passed
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Mar 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cfc4n cfc4n cfc4n approved these changes

Assignees
No one assigned
Labels
improve lgtm This PR has been approved by a maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yuweizzz @cfc4n