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
Triple protocol http1 upgrade support #14026
base: 3.3
Are you sure you want to change the base?
Conversation
3b48127
to
8238e49
Compare
...riple/src/main/java/org/apache/dubbo/rpc/protocol/tri/h12/HttpServerAfterUpgradeHandler.java
Outdated
Show resolved
Hide resolved
@oxsean PTAL |
8238e49
to
6d62b56
Compare
Please look into whether Application-Layer Protocol Negotiation is supported in addition to h2c: |
...pc/dubbo-rpc-triple/src/main/java/org/apache/dubbo/rpc/protocol/tri/TripleHttp2Protocol.java
Outdated
Show resolved
Hide resolved
Because you specified a http scheme (and not https), and asked curl to use HTTP/2, then curl will attempt to perform a HTTP/1.1 upgrade to HTTP/2, as can be seen from the logs. Typical HTTP/1.1 upgrades are performed using GET, not POST, notably the HTTP/1.1 upgrade to WebSocket. The server does not seem to be prepared to accept a POST with a body as an attempt to upgrade and replies with 413 because it does not expect a body. If you try a GET without body it will likely succeed. Alternatively, if you know that port 8889 accepts prior-knowledge clear-text HTTP/2 (that is, you can send directly HTTP/2 bytes to that port without having to perform a HTTP/1.1 upgrade), you can try:
`$ curl -v --http2-prior-knowledge -X POST -k -H "content-type:application/json" -d '["myFirstParameter"]' http://127.0.0.1:50052/org.apache.dubbo.springboot.demo.DemoService/sayHello
< HTTP/2 200
|
There is no documentation mentioned that methods other than GET are not supported. I think it might be because you haven't set maxContentLength --http2-prior-knowledge will skip negotiation, therefore this test is meaningless. |
The current common implementation of ALPN is through extended TLS. TLS processing in Dubbo is in ‘org.apache.dubbo.remoting.transport.netty4.NettyPortUnificationServerHandler’, before confirming the RPC protocol. Extensions to this implementation will have an impact on all RPC protocols. Could you please confirm whether support is needed at this level? |
I think so, but we need to confirm whether adding ALPN support cause a regression in protocols other than Triple, such as the Dubbo protocol. |
6d62b56
to
2ac7b09
Compare
You are right, thanks for the guidance. I have submitted a fix |
OK, I will submit a commit try to implement it. |
Submitted a commit. Please take a look, thanks. My test: |
LGTM, but have you tested no impact to dubbo protocol? |
Submitted some test cases to apache/dubbo-integration-cases#22
|
@AlbumenJ The problem has been solved, PTAL. |
.../main/java/org/apache/dubbo/remoting/transport/netty4/NettyPortUnificationServerHandler.java
Outdated
Show resolved
Hide resolved
...pc/dubbo-rpc-triple/src/main/java/org/apache/dubbo/rpc/protocol/tri/TripleHttp2Protocol.java
Outdated
Show resolved
Hide resolved
...pc/dubbo-rpc-triple/src/main/java/org/apache/dubbo/rpc/protocol/tri/TripleHttp2Protocol.java
Outdated
Show resolved
Hide resolved
...pc/dubbo-rpc-triple/src/main/java/org/apache/dubbo/rpc/protocol/tri/TripleHttp2Protocol.java
Outdated
Show resolved
Hide resolved
@walklown Sorry, I forgot commit the comments, please take a look. |
Submitted, thanks for your guidance ! |
} | ||
byte[] preface = new byte[in.readableBytes()]; | ||
in.readBytes(preface); | ||
Set<String> supported = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should cache this object in advance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your guidance.
But this is not a newly submitted code, all I did was separate it into a separate method for reuse. The record is submitted because the plug-in formatted it.
And this place will only be reached in the exception case that the appropriate protocol is not matched. Is it really a good idea to cache it in advance?
If we don't need to get the latest data, can I use 'protocols' directly? It comes from 'AbstractPortUnificationServer'.
aab8c3e
to
6016080
Compare
Quality Gate passedIssues Measures |
What is the purpose of the change
Triple protocol http1 upgrade support. For issue [Feature][3.3] Triple protocol http1 upgrade support
Brief changelog
Add Http2ServerUpgradeCodec to http1 channel handlers.
Verifying this change
https://github.com/walklown/dubbo-pr-test/tree/main/pr-test-triple