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.

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

optimize: support http1 automatic keepalive setting #15019

Merged
merged 4 commits into from
Dec 26, 2024
Merged

Conversation

funky-eyes
Copy link
Contributor

What is the purpose of the change?

由于ab压测工具默认是短连接,当使用该工具压测时,针对短连接请求,没有将对应的channel设置该ChannelFutureListener.CLOSE listener时,将使ab压测工具进程hang住,故增加自动识别请求是否需要保持长连接来解决该问题。

Since the ab benchmarking tool uses short connections by default, when using this tool for benchmarking, if the corresponding channel is not set with the ChannelFutureListener.CLOSE listener for short connection requests, it will cause the ab benchmarking tool process to hang. Therefore, adding automatic recognition of whether the request needs to keep the connection alive can solve this problem.

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

codecov-commenter commented Dec 25, 2024

Codecov Report

Attention: Patch coverage is 12.50000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 60.76%. Comparing base (a9e4910) to head (a6dfb8e).

Files with missing lines Patch % Lines
...bbo/remoting/http12/netty4/h1/NettyHttp1Codec.java 0.00% 7 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##                3.3   #15019      +/-   ##
============================================
- Coverage     60.76%   60.76%   -0.01%     
- Complexity    10866    10868       +2     
============================================
  Files          1882     1882              
  Lines         85983    85990       +7     
  Branches      12876    12878       +2     
============================================
+ Hits          52246    52250       +4     
- Misses        28289    28295       +6     
+ Partials       5448     5445       -3     
Flag Coverage Δ
integration-tests 33.00% <12.50%> (-0.13%) ⬇️
samples-tests 29.22% <12.50%> (-0.01%) ⬇️
unit-tests 58.92% <12.50%> (+0.01%) ⬆️

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.

import io.netty.handler.codec.http.HttpVersion;
import io.netty.handler.codec.http.LastHttpContent;

public class NettyHttp1Codec extends ChannelDuplexHandler {

boolean keepAlive;
Copy link
Collaborator

Choose a reason for hiding this comment

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

private boolean keepAlive;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

private boolean keepAlive;

done

}

private void doWriteMessage(ChannelHandlerContext ctx, HttpOutputMessage msg, ChannelPromise promise) {
if (HttpOutputMessage.EMPTY_MESSAGE == msg) {
ctx.writeAndFlush(LastHttpContent.EMPTY_LAST_CONTENT, promise);
if (!keepAlive) {
Copy link
Collaborator

@oxsean oxsean Dec 25, 2024

Choose a reason for hiding this comment

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

Not flipping the boolean will make the code better understood:

            if (keepAlive) {
                ctx.writeAndFlush(LastHttpContent.EMPTY_LAST_CONTENT, promise);
            } else {
                ctx.writeAndFlush(LastHttpContent.EMPTY_LAST_CONTENT, promise).addListener(ChannelFutureListener.CLOSE);
            }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not flipping the boolean will make the code better understood:

            if (keepAlive) {
                ctx.writeAndFlush(LastHttpContent.EMPTY_LAST_CONTENT, promise);
            } else {
                ctx.writeAndFlush(LastHttpContent.EMPTY_LAST_CONTENT, promise).addListener(ChannelFutureListener.CLOSE);
            }

done

@funky-eyes funky-eyes requested a review from oxsean December 26, 2024 05:35
@oxsean
Copy link
Collaborator

oxsean commented Dec 26, 2024

Could you please test whether SSE is work fine?
https://developer.mozilla.org/en-US/docs/Web/API/Server-sent_events/Using_server-sent_events

@funky-eyes
Copy link
Contributor Author

Could you please test whether SSE is work fine? https://developer.mozilla.org/en-US/docs/Web/API/Server-sent_events/Using_server-sent_events

In Spring, i can use SseEmitter to send messages, but how should this be done in Dubbo? Are there any related examples?

@oxsean oxsean merged commit a8b53a2 into apache:3.3 Dec 26, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants