ensure the side which enbale SO_LINGER and call showdownOutput to start TCP half-closure in fin_wait2 state can still receive and process the data which is send by another side in the close_wait state #11982

huibinliupush · 2022-01-08T12:47:10Z

Motivation:

we can not call doDeregister to cancel the key of the channel from the selector in the io.netty.channel.AbstractChannel.AbstractUnsafe#shutdownOutput(io.netty.channel.ChannelPromise, java.lang.Throwable) method。

because we should ensure the side which enbale SO_LINGER and call showdownOutput to start TCP half-closure in fin_wait2 state can still receive and process the data which is send by another side in the close_wait state。
and the shutdown function does not block regardless of the SO_LINGER setting on the socket,so we don't need to use GlobalEventExecutor to execute the shutdown

Modification:

In summary there is no need to call prepareToClose() in the AbstractChannel.AbstractUnsafe#shutdownOutput method

Result:

Fixes issue #11981

client in the FIN_WAIT2 state can read and process these data which is sended by server in the CLOSE_WAIT state，when SO_LINGGER is used in the TCP half-closed scenario

…rt TCP half-closure in fin_wait2 state can still receive and process the data which is send by another side in the close_wait state。

normanmaurer

A few comments :)

testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketHalfClosedTest.java

transport/src/main/java/io/netty/channel/AbstractChannel.java

normanmaurer · 2022-01-10T13:17:35Z

@huibinliupush did you sigh our ICLA yet ?

huibinliupush · 2022-01-10T14:34:27Z

@huibinliupush did you sigh our ICLA yet ?

@normanmaurer yeah I already signed it before
I will solve the problem of the unit test you commented tomorrow，because it's already midnight in china

Co-authored-by: Norman Maurer <norman_maurer@apple.com>

- delete codec in the test channel pipeline - use CountdownLatch to wait the halfClosure to finish - call ReferenceCountUtil.release(msg) in the channelRead handler - follow the same structure for tests, ensures the test will be run for NIO,OIO,Epoll and Kqueue

testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketHalfClosedTest.java

transport/src/main/java/io/netty/channel/AbstractChannel.java

…cketHalfClosedTest.java Co-authored-by: Norman Maurer <norman_maurer@apple.com>

normanmaurer · 2022-01-11T09:58:32Z

@huibinliupush please fix check style error:

Error:  /home/runner/work/netty/netty/transport/src/main/java/io/netty/channel/AbstractChannel.java:654: Line is longer than 120 characters (found 135). [LineLength]
901
Error:  /home/runner/work/netty/netty/transport/src/main/java/io/netty/channel/AbstractChannel.java:655: Line is longer than 120 characters (found 162). [LineLength]
902
Error:  /home/runner/work/netty/netty/transport/src/main/java/io/netty/channel/AbstractChannel.java:658: Line is longer than 120 characters (found 129). [LineLength]

huibinliupush · 2022-01-11T10:21:59Z

@huibinliupush please fix check style error:

Error:  /home/runner/work/netty/netty/transport/src/main/java/io/netty/channel/AbstractChannel.java:654: Line is longer than 120 characters (found 135). [LineLength]
901
Error:  /home/runner/work/netty/netty/transport/src/main/java/io/netty/channel/AbstractChannel.java:655: Line is longer than 120 characters (found 162). [LineLength]
902
Error:  /home/runner/work/netty/netty/transport/src/main/java/io/netty/channel/AbstractChannel.java:658: Line is longer than 120 characters (found 129). [LineLength]

@normanmaurer fix it

normanmaurer · 2022-01-11T10:38:46Z

@huibinliupush I think we are almost there..

Please also fix these:

Error:  /home/runner/work/netty/netty/testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketHalfClosedTest.java:65: two or more consecutive empty lines [RegexpMultiline]
5205
Error:  /home/runner/work/netty/netty/testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketHalfClosedTest.java:69: Line is longer than 120 characters (found 128). [LineLength]
5206
Error:  /home/runner/work/netty/netty/testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketHalfClosedTest.java:87:80: '{' is not preceded with whitespace. [WhitespaceAround]
5207
Error:  /home/runner/work/netty/netty/testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketHalfClosedTest.java:91:71: 'typecast' is not followed by whitespace. [WhitespaceAfter]
5208
Error:  /home/runner/work/netty/netty/testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketHalfClosedTest.java:101: an empty line before } [RegexpMultiline]
5209
Error:  /home/runner/work/netty/netty/testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketHalfClosedTest.java:102: two or more consecutive empty lines [RegexpMultiline]
5210
Error:  /home/runner/work/netty/netty/testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketHalfClosedTest.java:110:79: '{' is not preceded with whitespace. [WhitespaceAround]

huibinliupush · 2022-01-11T12:39:09Z

@huibinliupush I think we are almost there..

Please also fix these:

Error:  /home/runner/work/netty/netty/testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketHalfClosedTest.java:65: two or more consecutive empty lines [RegexpMultiline]
5205
Error:  /home/runner/work/netty/netty/testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketHalfClosedTest.java:69: Line is longer than 120 characters (found 128). [LineLength]
5206
Error:  /home/runner/work/netty/netty/testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketHalfClosedTest.java:87:80: '{' is not preceded with whitespace. [WhitespaceAround]
5207
Error:  /home/runner/work/netty/netty/testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketHalfClosedTest.java:91:71: 'typecast' is not followed by whitespace. [WhitespaceAfter]
5208
Error:  /home/runner/work/netty/netty/testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketHalfClosedTest.java:101: an empty line before } [RegexpMultiline]
5209
Error:  /home/runner/work/netty/netty/testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketHalfClosedTest.java:102: two or more consecutive empty lines [RegexpMultiline]
5210
Error:  /home/runner/work/netty/netty/testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketHalfClosedTest.java:110:79: '{' is not preceded with whitespace. [WhitespaceAround]

@normanmaurer my intellij checksytle plugin reported error but did not prompt specific error information, so it took me some time，sorry about that

normanmaurer · 2022-01-11T15:56:18Z

@huibinliupush thanks a lot !

…rt TCP half-closure in fin_wait2 state can still receive and process the data which is send by another side in the close_wait state (#11982) Motivation: we can not call `doDeregister` to cancel the key of the channel from the selector in the `io.netty.channel.AbstractChannel.AbstractUnsafe#shutdownOutput(io.netty.channel.ChannelPromise, java.lang.Throwable)` method。 - because we should ensure the side which enbale `SO_LINGER` and call `showdownOutput` to start TCP half-closure in fin_wait2 state can still receive and process the data which is send by another side in the close_wait state。 - and the shutdown function does not block regardless of the SO_LINGER setting on the socket,so we don't need to use GlobalEventExecutor to execute the shutdown Modification: In summary there is no need to call `prepareToClose()` in the `AbstractChannel.AbstractUnsafe#shutdownOutput` method Result: Fixes issue #11981 client in the FIN_WAIT2 state can read and process these data which is sended by server in the CLOSE_WAIT state，when SO_LINGGER is used in the TCP half-closed scenario Co-authored-by: Norman Maurer <norman_maurer@apple.com>

huibinliupush · 2022-01-11T16:18:17Z

@huibinliupush thanks a lot !

@normanmaurer so grateful to have learned so much from you，also thank you very much for reviewing this PR

…rt TCP half-closure in fin_wait2 state can still receive and process the data which is send by another side in the close_wait state (netty#11982) Motivation: we can not call `doDeregister` to cancel the key of the channel from the selector in the `io.netty.channel.AbstractChannel.AbstractUnsafe#shutdownOutput(io.netty.channel.ChannelPromise, java.lang.Throwable)` method。 - because we should ensure the side which enbale `SO_LINGER` and call `showdownOutput` to start TCP half-closure in fin_wait2 state can still receive and process the data which is send by another side in the close_wait state。 - and the shutdown function does not block regardless of the SO_LINGER setting on the socket,so we don't need to use GlobalEventExecutor to execute the shutdown Modification: In summary there is no need to call `prepareToClose()` in the `AbstractChannel.AbstractUnsafe#shutdownOutput` method Result: Fixes issue netty#11981 client in the FIN_WAIT2 state can read and process these data which is sended by server in the CLOSE_WAIT state，when SO_LINGGER is used in the TCP half-closed scenario Co-authored-by: Norman Maurer <norman_maurer@apple.com>

huibinliupush added 2 commits January 8, 2022 20:40

ensure the side which enbale SO_LINGER and call showdownOutput to sta…

d099007

…rt TCP half-closure in fin_wait2 state can still receive and process the data which is send by another side in the close_wait state。

ensure the side which enbale SO_LINGER and call showdownOutput to sta…

197547c

…rt TCP half-closure in fin_wait2 state can still receive and process the data which is send by another side in the close_wait state。

huibinliupush changed the title ~~So linger halfclosure issue~~ ensure the side which enbale SO_LINGER and call showdownOutput to start TCP half-closure in fin_wait2 state can still receive and process the data which is send by another side in the close_wait state Jan 8, 2022

normanmaurer requested changes Jan 10, 2022

View reviewed changes

normanmaurer added this to the 4.1.73.Final milestone Jan 10, 2022

huibinliupush and others added 4 commits January 10, 2022 22:42

optimize unit tests

4d56b25

- delete codec in the test channel pipeline - use CountdownLatch to wait the halfClosure to finish - call ReferenceCountUtil.release(msg) in the channelRead handler - follow the same structure for tests, ensures the test will be run for NIO,OIO,Epoll and Kqueue

huibinliupush requested a review from normanmaurer January 11, 2022 09:06

normanmaurer requested changes Jan 11, 2022

View reviewed changes

transport/src/main/java/io/netty/channel/AbstractChannel.java Show resolved Hide resolved

huibinliupush and others added 8 commits January 11, 2022 17:16

remove wildcard imports in unit test

ba81349

huibinliupush requested a review from normanmaurer January 11, 2022 09:54

fix check style errors in AbstracChannel

5d8963f

normanmaurer added 3 commits January 11, 2022 13:05

Cleanup test

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

54515ef

Update SocketHalfClosedTest.java

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

c0f2027

Create SocketHalfClosedTest.java

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

f487da2

huibinliupush closed this Jan 11, 2022

huibinliupush reopened this Jan 11, 2022

normanmaurer approved these changes Jan 11, 2022

View reviewed changes

normanmaurer merged commit d60ea59 into netty:4.1 Jan 11, 2022

huibinliupush deleted the so_linger-halfclosure-issue branch January 11, 2022 23:52

ensure the side which enbale SO_LINGER and call showdownOutput to start TCP half-closure in fin_wait2 state can still receive and process the data which is send by another side in the close_wait state #11982

ensure the side which enbale SO_LINGER and call showdownOutput to start TCP half-closure in fin_wait2 state can still receive and process the data which is send by another side in the close_wait state #11982

Conversation

huibinliupush commented Jan 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

normanmaurer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

normanmaurer commented Jan 10, 2022

Uh oh!

huibinliupush commented Jan 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

normanmaurer commented Jan 11, 2022

Uh oh!

huibinliupush commented Jan 11, 2022

Uh oh!

normanmaurer commented Jan 11, 2022

Uh oh!

huibinliupush commented Jan 11, 2022

Uh oh!

normanmaurer commented Jan 11, 2022

Uh oh!

huibinliupush commented Jan 11, 2022

Uh oh!

huibinliupush commented Jan 8, 2022 •

edited

Loading

huibinliupush commented Jan 10, 2022 •

edited

Loading