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

TCP_USER_TIMEOUT property setting issue #11517

Closed
hlx502 opened this issue Sep 9, 2024 · 5 comments · Fixed by #11564
Closed

TCP_USER_TIMEOUT property setting issue #11517

hlx502 opened this issue Sep 9, 2024 · 5 comments · Fixed by #11564
Milestone

Comments

@hlx502
Copy link
Contributor

hlx502 commented Sep 9, 2024

The channelFactory may create a NioSocketChannel, but here it uses Utils.maybeGetTcpUserTimeoutOption() to determine whether to set the TCP_USER_TIMEOUT. It may lead to setting TCP_USER_TIMEOUT when using NioSocketChannel, but in fact, only EpollSocketChannel supports setting this property. I feel that the judgment here is not strict enough. Should we consider optimization?
image

@ejona86
Copy link
Member

ejona86 commented Sep 11, 2024

I'd expect NioSocketChannel to ignore the unknown (to it) option. Have you seen any error caused by this?

@hlx502 hlx502 closed this as completed Sep 13, 2024
@hlx502
Copy link
Contributor Author

hlx502 commented Sep 13, 2024

During the testing of ETCD, nio was used. But it seems that the call stack has set TCP_USER_TIMEOUT. There are no errors, only the warn log below will be generated. Can we add a determination of the current channel type in NettyClientTransport to avoid setting TCP_USER_TIMEOUT
log:
image
stack:
img_v3_02ef_53e27550-787e-481b-946e-db2ffc40c4dh
img_v3_02ef_2f10e760-1e04-4165-865a-c6274b998d2h

@ejona86
Copy link
Member

ejona86 commented Sep 13, 2024

Ah, okay. It would be good to avoid the warning. Although we don't know the type returned by the channelFactory, so it may be a bit hard to figure out which case we are in. Do you have any ideas on how to fix it? We could choose to avoid TCP_USER_TIMEOUT if someone sets the channelFactory on NettyChannelBuilder.

@ejona86 ejona86 reopened this Sep 13, 2024
@hlx502
Copy link
Contributor Author

hlx502 commented Sep 18, 2024

It seems that modifying NettyChannelBuilder is not a good solution. Maybe we can set the TCP_USER_TIMEOUT attribute after the channel is registered and before the channel is connected. If it's feasible, I'll file a PR to fix it.
image

@ejona86
Copy link
Member

ejona86 commented Oct 3, 2024

I honestly don't know if that works. We could do it before connect(), which is good. But there's two risks:

  1. it isn't running on the event loop
  2. the channel might not notice the change in option

Looking at the code, it seems Netty directly modifies the socket when called and socket is final. https://github.com/netty/netty/blob/7679b9efdec71b54d06db91eef40ad913681294a/transport-classes-epoll/src/main/java/io/netty/channel/epoll/EpollSocketChannelConfig.java#L161

Yeah, seems it could work.

ejona86 pushed a commit that referenced this issue Oct 22, 2024
In NettyClientTransport, the TCP_USER_TIMEOUT attribute can be set only
if the channel is of the AbstractEpollStreamChannel.

Fixes #11517
@ejona86 ejona86 added this to the 1.68 milestone Oct 22, 2024
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 a pull request may close this issue.

2 participants