[3.3] Add connection preface process to TripleHttp2Protocol #15368

zrlw · 2025-05-09T11:47:18Z

What is the purpose of the change?

fixed #15233 No provider available issue, the roots of the issue is client sent large headers frame before receiving server connection preface (http2 settings frame), which cause server sent back go away frame and the connection was disconnected by the server.

see details at https://httpwg.org/specs/rfc7540.html#ConnectionHeader

3.5. HTTP/2 Connection Preface
In HTTP/2, each endpoint is required to send a connection preface as a final confirmation of the protocol in use and to establish the initial settings for the HTTP/2 connection. The client and server each send a different connection preface.
The client connection preface starts with a sequence of 24 octets, which in hex notation is:
0x505249202a20485454502f322e300d0a0d0a534d0d0a0d0a
That is, the connection preface starts with the string PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n). This sequence MUST be followed by a SETTINGS frame (Section 6.5), which MAY be empty. 
... ...
The server connection preface consists of a potentially empty SETTINGS frame (Section 6.5) that MUST be the first frame the server sends in the HTTP/2 connection.

https://quafoo.gitbooks.io/http2-rfc7540-zh-cn-en/content/chapter3/section3.5.html

HTTP/2 Connection Preface / HTTP/2连接前奏
在HTTP/2中，要求两端都要发送一个连接前奏，作为对所使用协议的最终确认，并确定HTTP/2连接的初始设置。客户端和服务端各自发送不同的连接前奏。
客户端连接前奏以一个24字节的序列开始，用十六进制表示为：
0x505249202a20485454502f322e300d0a0d0a534d0d0a0d0a
即，连接前奏以字符串"PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n"开始。这个序列后面必须跟一个可以为空的 SETTINGS 帧( 6.5节 )。
... ...
服务端连接前奏包含一个可能为空的 SETTINGS 帧( 6.5节 )，它必须由服务端在HTTP/2连接中首先发送。

Checklist

Make sure there is a GitHub_issue field for the change.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Write necessary unit-test to verify your logic correction. If the new feature or significant change is committed, please remember to add sample in dubbo samples project.
Make sure gitHub actions can pass. Why the workflow is failing and how to fix it?

codecov-commenter · 2025-05-09T12:05:51Z

Codecov Report

Attention: Patch coverage is 70.45455% with 13 lines in your changes missing coverage. Please review.

Project coverage is 60.91%. Comparing base (84d8cc2) to head (05d2cd2).
Report is 74 commits behind head on 3.3.

Files with missing lines	Patch %	Lines
...ransport/netty4/AbstractNettyConnectionClient.java	60.71%	8 Missing and 3 partials ⚠️
...oting/transport/netty4/NettyConnectionHandler.java	50.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##                3.3   #15368      +/-   ##
============================================
+ Coverage     60.74%   60.91%   +0.17%     
- Complexity    10913    11439     +526     
============================================
  Files          1886     1889       +3     
  Lines         86145    86310     +165     
  Branches      12906    12935      +29     
============================================
+ Hits          52331    52580     +249     
+ Misses        28359    28288      -71     
+ Partials       5455     5442      -13

Flag	Coverage Δ
integration-tests	`?`
integration-tests-java17	`33.11% <61.36%> (?)`
integration-tests-java8	`33.15% <61.36%> (?)`
samples-tests	`?`
samples-tests-java17	`31.42% <63.63%> (?)`
samples-tests-java8	`29.29% <52.27%> (?)`
unit-tests	`58.85% <61.36%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

AlbumenJ

@oxsean PTAL

zrlw · 2025-06-07T02:44:12Z

refer to okhttp:

Http2.java
  static final ByteString CONNECTION_PREFACE
      = ByteString.encodeUtf8("PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n");

Http2Reader.java
  public void readConnectionPreface(Handler handler) throws IOException {
    if (client) {
      // The client reads the initial SETTINGS frame.
      if (!nextFrame(true, handler)) {
        throw ioException("Required SETTINGS preface not received");
      }
    } else {
      // The server reads the CONNECTION_PREFACE byte string.
      ByteString connectionPreface = source.readByteString(CONNECTION_PREFACE.size());
      if (logger.isLoggable(FINE)) logger.fine(format("<< CONNECTION %s", connectionPreface.hex()));
      if (!CONNECTION_PREFACE.equals(connectionPreface)) {
        throw ioException("Expected a connection header but was %s", connectionPreface.utf8());
      }
    }
  }

oxsean · 2025-06-07T09:35:37Z

.../src/main/java/org/apache/dubbo/remoting/transport/netty4/AbstractNettyConnectionClient.java

            throw remotingException;
        }
+
+        if (connectionPrefaceReceivedPromise != null) {


Why not handle it in connect? I think it's not appropriate to do it in initBootstrap

oxsean · 2025-06-07T09:37:21Z

Sorry for late, But I think it's better to merge the PR after the review.

oxsean · 2025-06-07T09:39:12Z

.../src/main/java/org/apache/dubbo/remoting/transport/netty4/AbstractNettyConnectionClient.java

+
    public static final AttributeKey<AbstractConnectionClient> CONNECTION = AttributeKey.valueOf("connection");

    public AbstractNettyConnectionClient(URL url, ChannelHandler handler) throws RemotingException {


I think this logic should be added to org.apache.dubbo.remoting.transport.netty4.NettyConnectionClient, because HTTP/3 probably doesn't need it."

oxsean · 2025-06-07T14:00:42Z

@zrlw @AlbumenJ I think we can just make some changes in NettyConnectionClient, move the TripleHttp2SettingsHandler setup there and deal with it — no need modify other classes.

…#15368)" This reverts commit e396bde.

…" (#15435) This reverts commit e396bde.

zrlw · 2025-06-08T17:41:19Z

this pr was reverted because i found it has problems which should be fixed, and i want to find a more elegant way to do it.

zrlw · 2025-06-09T00:34:56Z

@zrlw @AlbumenJ I think we can just make some changes in NettyConnectionClient, move the TripleHttp2SettingsHandler setup there and deal with it — no need modify other classes.

how to know the protol of current NettyConnectionClient is http2?

zrlw · 2025-06-09T05:24:23Z

submit a new pr: #15436

zrlw force-pushed the 3.3-fixed-TripleHttp2Protocol branch 2 times, most recently from 8fec5d3 to 0ce80d2 Compare May 9, 2025 14:03

zrlw changed the title ~~[3.3] Wait http2 remote settings at doConnect~~ [3.3] Wait http2 remote settings at doConnect if protocol is TripleHttp2Protocol May 9, 2025

zrlw force-pushed the 3.3-fixed-TripleHttp2Protocol branch from 0ce80d2 to 25a8d11 Compare May 9, 2025 17:15

zrlw changed the title ~~[3.3] Wait http2 remote settings at doConnect if protocol is TripleHttp2Protocol~~ [3.3] Wait remote settings at method 'doConnect' if protocol needs it May 9, 2025

zrlw force-pushed the 3.3-fixed-TripleHttp2Protocol branch from 25a8d11 to 8cde57a Compare May 9, 2025 18:12

zrlw changed the title ~~[3.3] Wait remote settings at method 'doConnect' if protocol needs it~~ [3.3] Wait for notification of the end of negotiation at 'doConnect' if protocol needs it May 9, 2025

zrlw force-pushed the 3.3-fixed-TripleHttp2Protocol branch 3 times, most recently from 90172cc to e99518b Compare May 11, 2025 10:12

AlbumenJ approved these changes May 12, 2025

View reviewed changes

oxsean self-requested a review May 15, 2025 14:59

zrlw changed the title ~~[3.3] Wait for notification of the end of negotiation at 'doConnect' if protocol needs it~~ [3.3] Wait for connection preface from http2 server Jun 7, 2025

Add connection preface process to TripleHttp2Protocol

05d2cd2

zrlw force-pushed the 3.3-fixed-TripleHttp2Protocol branch from 74e2ce0 to 05d2cd2 Compare June 7, 2025 03:27

zrlw changed the title ~~[3.3] Wait for connection preface from http2 server~~ [3.3] Add connection preface process to TripleHttp2Protocol Jun 7, 2025

zrlw merged commit e396bde into apache:3.3 Jun 7, 2025
29 checks passed

zrlw deleted the 3.3-fixed-TripleHttp2Protocol branch June 7, 2025 05:07

oxsean reviewed Jun 7, 2025

View reviewed changes

zrlw added a commit to zrlw/dubbo that referenced this pull request Jun 8, 2025

Revert "Add connection preface process to TripleHttp2Protocol (apache…

6152c53

…#15368)" This reverts commit e396bde.

zrlw added a commit that referenced this pull request Jun 8, 2025

Revert "Add connection preface process to TripleHttp2Protocol (#15368)…

79683be

…" (#15435) This reverts commit e396bde.

zrlw mentioned this pull request Jun 9, 2025

[3.3] Add http2 client connection preface process #15436

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[3.3] Add connection preface process to TripleHttp2Protocol #15368

[3.3] Add connection preface process to TripleHttp2Protocol #15368

Uh oh!

zrlw commented May 9, 2025 •

edited

Loading

Uh oh!

codecov-commenter commented May 9, 2025 •

edited

Loading

Uh oh!

AlbumenJ left a comment

Uh oh!

zrlw commented Jun 7, 2025

Uh oh!

Uh oh!

oxsean Jun 7, 2025

Uh oh!

oxsean commented Jun 7, 2025

Uh oh!

oxsean Jun 7, 2025

Uh oh!

oxsean commented Jun 7, 2025

Uh oh!

zrlw commented Jun 8, 2025

Uh oh!

zrlw commented Jun 9, 2025

Uh oh!

zrlw commented Jun 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants


		public static final AttributeKey<AbstractConnectionClient> CONNECTION = AttributeKey.valueOf("connection");

		public AbstractNettyConnectionClient(URL url, ChannelHandler handler) throws RemotingException {

[3.3] Add connection preface process to TripleHttp2Protocol #15368

[3.3] Add connection preface process to TripleHttp2Protocol #15368

Uh oh!

Conversation

zrlw commented May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the purpose of the change?

Checklist

Uh oh!

codecov-commenter commented May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

AlbumenJ left a comment

Choose a reason for hiding this comment

Uh oh!

zrlw commented Jun 7, 2025

Uh oh!

Uh oh!

oxsean Jun 7, 2025

Choose a reason for hiding this comment

Uh oh!

oxsean commented Jun 7, 2025

Uh oh!

oxsean Jun 7, 2025

Choose a reason for hiding this comment

Uh oh!

oxsean commented Jun 7, 2025

Uh oh!

zrlw commented Jun 8, 2025

Uh oh!

zrlw commented Jun 9, 2025

Uh oh!

zrlw commented Jun 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zrlw commented May 9, 2025 •

edited

Loading

codecov-commenter commented May 9, 2025 •

edited

Loading