Skip to content

[3.3] Add connection preface process to TripleHttp2Protocol#15368

Merged
zrlw merged 1 commit intoapache:3.3from
zrlw:3.3-fixed-TripleHttp2Protocol
Jun 7, 2025
Merged

[3.3] Add connection preface process to TripleHttp2Protocol#15368
zrlw merged 1 commit intoapache:3.3from
zrlw:3.3-fixed-TripleHttp2Protocol

Conversation

@zrlw
Copy link
Copy Markdown
Contributor

@zrlw zrlw commented May 9, 2025 •
edited
Loading

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.
go away issue
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
Copy link
Copy Markdown

codecov-commenter commented May 9, 2025 •
edited
Loading

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.

@zrlw zrlw force-pushed the 3.3-fixed-TripleHttp2Protocol branch 2 times, most recently from 8fec5d3 to 0ce80d2 Compare May 9, 2025 14:03
@zrlw 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 zrlw force-pushed the 3.3-fixed-TripleHttp2Protocol branch from 0ce80d2 to 25a8d11 Compare May 9, 2025 17:15
@zrlw 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 zrlw force-pushed the 3.3-fixed-TripleHttp2Protocol branch from 25a8d11 to 8cde57a Compare May 9, 2025 18:12
@zrlw 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 zrlw force-pushed the 3.3-fixed-TripleHttp2Protocol branch 3 times, most recently from 90172cc to e99518b Compare May 11, 2025 10:12
AlbumenJ
AlbumenJ approved these changes May 12, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@AlbumenJ AlbumenJ left a comment

Choose a reason for hiding this comment

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

@oxsean PTAL

@oxsean oxsean self-requested a review May 15, 2025 14:59
@zrlw 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
@zrlw
Copy link
Copy Markdown
Contributor Author

zrlw commented Jun 7, 2025

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());
      }
    }
  }

@zrlw zrlw force-pushed the 3.3-fixed-TripleHttp2Protocol branch from 74e2ce0 to 05d2cd2 Compare June 7, 2025 03:27
@zrlw 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 zrlw merged commit e396bde into apache:3.3 Jun 7, 2025
29 checks passed
@zrlw zrlw deleted the 3.3-fixed-TripleHttp2Protocol branch June 7, 2025 05:07
oxsean
oxsean reviewed Jun 7, 2025
View reviewed changes
Comment thread .../src/main/java/org/apache/dubbo/remoting/transport/netty4/AbstractNettyConnectionClient.java
throw remotingException;
}

if (connectionPrefaceReceivedPromise != null) {
Copy link
Copy Markdown
Contributor

@oxsean oxsean Jun 7, 2025

Choose a reason for hiding this comment

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

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

@oxsean
Copy link
Copy Markdown
Contributor

oxsean commented Jun 7, 2025

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

oxsean
oxsean reviewed Jun 7, 2025
View reviewed changes
Comment thread .../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 {
Copy link
Copy Markdown
Contributor

@oxsean oxsean Jun 7, 2025

Choose a reason for hiding this comment

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

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

@oxsean
Copy link
Copy Markdown
Contributor

oxsean commented Jun 7, 2025

@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.

zrlw added a commit to zrlw/dubbo that referenced this pull request Jun 8, 2025
@zrlw
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
@zrlw
Revert "Add connection preface process to TripleHttp2Protocol (#15368)…
79683be 
…" (#15435)

This reverts commit e396bde.
@zrlw
Copy link
Copy Markdown
Contributor Author

zrlw commented Jun 8, 2025

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
Copy link
Copy Markdown
Contributor Author

zrlw commented Jun 9, 2025

@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 zrlw mentioned this pull request Jun 9, 2025
Merged
4 tasks
@zrlw
Copy link
Copy Markdown
Contributor Author

zrlw commented Jun 9, 2025

submit a new pr: #15436

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oxsean oxsean oxsean left review comments

@AlbumenJ AlbumenJ AlbumenJ approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug] No provider available due to receiving go away frame immediately after successful connection

4 participants

@zrlw @codecov-commenter @oxsean @AlbumenJ