Close Channel and fail bootstrap when setting a ChannelOption causes …#15896
Conversation
|
I think we should also cherry-pick this into 4.1 |
…an error Motivation: We should close the Channel and fail the future of the bootstrap if during setting a ChannelOption we observe an error. At the moment we just log which might make things hard to debug. Modifications: - Adjust code to also close and faile the future - Add testcases Result: Related to #15860 (comment)
d626ac1 to
118067b
Compare
|
Debugging is not really the issue. The main problem is that Netty will continue even if one or more of the channel options couldn't be set. Depending on the channel option this may have undesired consequences. |
|
@WhiteTrashLord reworded... Thanks |
|
practical , but may cause problem in production，there are many wrong configured jars |
Yeah, actually I was a bit surprised that Norman accepted this change so eagerly :D. This change makes a lot of sense and I really need it. But there is a high chance that it will break things for some users who have ignored warnings so far. Maybe this can be the default behavior in Netty 5. For me it would be okay to just add a System property like io.netty.channel.throwChannelOptionException in Netty 4.1 and 4.2. io.netty.channel.throwChannelOptionException would be false by default The code would look like this: if ( PROP_THROWCHANNELOPTIONEXCEPTION == true ) |
|
@WhiteTrashLord honestly there should be no breakage and if there is it is for a good reason imho |
|
@chrisvest @yawkat @franz1981 @bryce-anderson do you think we should better add a property to fallback to the old behavior so people can set it when they run into issues? I still think the default should be changed to what we have here now tho. |
|
And also @trustin ^^ |
|
It's 4.2 so I would:
This in 3-4 ticks releases |
|
If you google for "Failed to set channel option" netty you can find a few results like https://groups.google.com/g/vertx/c/pR89FCWeYJA I would do it like this: |
I agree with this. Add a property to revert to 4.1 behavior. I don't think we need to stagger the change of the default, because no one will pay attention to the changed default until it impacts them, anyway. The few teams who do pay attention will notice our call-out in the release notes. |
|
Added a property to restore old behavior but close on failure by default |
#15896) …an error Motivation: We should close the Channel and fail the future of the bootstrap if during setting a ChannelOption we observe an error. At the moment we just log which might make things hard to debug and leave the Channel in an unexpected state. People can go back to the old behavior by using `-Dio.netty.bootstrap.closeOnSetOptionFailure=false`. Modifications: - Adjust code to also close and faile the future - Add testcases Result: Related to #15860 (comment)
#15970) …… (#15896) …an error Motivation: We should close the Channel and fail the future of the bootstrap if during setting a ChannelOption we observe an error. At the moment we just log which might make things hard to debug and leave the Channel in an unexpected state. People can go back to the old behavior by using `-Dio.netty.bootstrap.closeOnSetOptionFailure=false`. Modifications: - Adjust code to also close and faile the future - Add testcases Result: Related to #15860 (comment)
#15896) …an error Motivation: We should close the Channel and fail the future of the bootstrap if during setting a ChannelOption we observe an error. At the moment we just log which might make things hard to debug and leave the Channel in an unexpected state. People can go back to the old behavior by using `-Dio.netty.bootstrap.closeOnSetOptionFailure=false`. Modifications: - Adjust code to also close and faile the future - Add testcases Result: Related to #15860 (comment)
#15972) …… (#15896) …an error Motivation: We should close the Channel and fail the future of the bootstrap if during setting a ChannelOption we observe an error. At the moment we just log which might make things hard to debug and leave the Channel in an unexpected state. People can go back to the old behavior by using `-Dio.netty.bootstrap.closeOnSetOptionFailure=false`. Modifications: - Adjust code to also close and faile the future - Add testcases Result: Related to #15860 (comment)
Motivation: `ChannelUtil.applyDefaultChannelOptions()` overflows when `idleTimeoutMillis` is near `Long.MAX_VALUE`. The expression `idleTimeoutMillis + TCP_USER_TIMEOUT_BUFFER_MILLIS` (where the buffer is 5,000) wraps the `long` type to a large negative number, and `Ints.saturatedCast()` then clamps it to `Integer.MIN_VALUE` (-2,147,483,648). Linux rejects this negative value for `TCP_USER_TIMEOUT` via `setsockopt()` with `EINVAL`. Starting with **Netty 4.1.131+**, channel option values are validated at set time and throw exceptions instead of being silently logged internally (see netty/netty#15896). This means the overflow now causes a hard failure rather than a silent misconfiguration. Modifications: - Use `LongMath.saturatedAdd` to prevent the `long` overflow before passing the result to `Ints.saturatedCast`: ```java final int tcpUserTimeoutMillis = Ints.saturatedCast(LongMath.saturatedAdd(idleTimeoutMillis, TCP_USER_TIMEOUT_BUFFER_MILLIS)); - Add a regression test that passes Long.MAX_VALUE as idleTimeoutMillis and asserts the result is Integer.MAX_VALUE. Result: - Long.MAX_VALUE idle timeout no longer overflows — produces Integer.MAX_VALUE (~24.8 day timeout) instead of Integer.MIN_VALUE (rejected by Linux). - For normal values (e.g., 2000), Math.min is a no-op and behavior is unchanged.
…an error
Motivation:
We should close the Channel and fail the future of the bootstrap if during setting a ChannelOption we observe an error. At the moment we just log which might make things hard to debug and leave the Channel in an unexpected state.
Modifications:
Result:
Related to #15860 (comment)