Shutting down the outbound side of the channel should not accept futu… #7041

Scottmitch · 2017-08-01T16:27:10Z

…re writes

Motivation:
Implementations of DuplexChannel delegate the shutdownOutput to the underlying transport, but do not take any action on the ChannelOutboundBuffer. In the event of a write failure due to the underlying transport failing and application may attempt to shutdown the output and allow the read side the transport to finish and detect the close. However this may result in an issue where writes are failed, this generates a writability change, we continue to write more data, and this may lead to another writability change, and this loop may continue. Shutting down the output should fail all pending writes and not allow any future writes to avoid this scenario.

Modifications:

Implementations of DuplexChannel should null out the ChannelOutboundBuffer and fail all pending writes

Result:
More controlled sequencing for shutting down the output side of a channel.

normanmaurer

Can you also add a testcase ?

normanmaurer · 2017-08-01T16:32:17Z

transport/src/main/java/io/netty/channel/socket/ChannelOutputShutdownException.java

+/**
+ * Used to fail pending writes when a channel's output has been shutdown.
+ */
+public class ChannelOutputShutdownException extends java.io.IOException {


Just import IOException?

normanmaurer · 2017-08-01T16:33:38Z

transport/src/main/java/io/netty/channel/socket/nio/NioSocketChannel.java

@@ -247,6 +247,7 @@ public void run() {
    private void shutdownOutput0(final ChannelPromise promise) {
        try {
            shutdownOutput0();


Can't we move the unsafe call to shutdownOutput0() ?

normanmaurer · 2017-08-01T16:34:40Z

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

@@ -663,6 +663,10 @@ public void run() {
        clearNioBuffers();
    }

+    void close(final ClosedChannelException cause) {
+        close(cause, false);


Nit: you can remove final

done (comment should be on the line above I'm guessing?)

Scottmitch · 2017-08-01T20:54:45Z

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

+            try {
+                Boolean writability = writabilityQueue.takeLast();
+                assertEquals(isWritable, writability);
+                // TODO(scott): why do we get multiple writability changes here ... race condition?


@normanmaurer - note that while writing outside the event loop I'm getting more writability change notifications than expected. If you would also only expect to get two notifications (not writable after the write, then writable after the flush) then I can open an issue and investigate as a followup.

please do and assign to me.

Scottmitch · 2017-08-01T20:56:00Z

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

+            ss.bind(newSocketAddress());
+            cb.option(ChannelOption.WRITE_BUFFER_WATER_MARK, new WriteBufferWaterMark(2, 4));
+            ch = (SocketChannel) cb.handler(h).connect(ss.getLocalSocketAddress()).sync().channel();
+            assumeFalse(ch instanceof OioSocketChannel);


OIO doesn't do writability notifications the same way

please do open an issue and assign to me.

Scottmitch · 2017-08-01T23:26:04Z

Build failure appears to be unrelated ... captured in #3138 (comment)

Scottmitch · 2017-08-02T02:42:02Z

Same build failure on a different host (Debian 7-1) https://garage.netty.io/teamcity/viewLog.html?buildId=22855&buildTypeId=pulls_netty&guest=1 ... not sure why this would be related yet ... needs more investigation.

netkins · 2017-08-02T07:33:01Z

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

+            try {
+                Boolean writability = writabilityQueue.takeLast();
+                assertEquals(isWritable, writability);
+                // TODO(scott): why do we get multiple writability changes here ... race condition?


Complete the task associated to this TODO comment.

netkins · 2017-08-02T07:33:02Z

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

+                // TODO(scott): why do we get multiple writability changes here ... race condition?
+                drainWritabilityQueue();
+            } catch (Throwable c) {
+                c.printStackTrace();


Use a logger to log this exception.

please address.

Scottmitch · 2017-08-02T20:09:17Z

no build failure this time ... kicking off another build.

Scottmitch · 2017-08-03T18:48:43Z

no build failure ... kicking off another build (round 2)

normanmaurer · 2017-08-03T19:34:35Z

transport/src/main/java/io/netty/channel/socket/ChannelOutputShutdownException.java

+/**
+ * Used to fail pending writes when a channel's output has been shutdown.
+ */
+public class ChannelOutputShutdownException extends IOException {


normanmaurer · 2017-08-03T19:35:00Z

transport/src/main/java/io/netty/channel/socket/ChannelOutputShutdownException.java

+/**
+ * Used to fail pending writes when a channel's output has been shutdown.
+ */
+public class ChannelOutputShutdownException extends IOException {


also do you want to add @UnstableApi ?

…re writes Motivation: Implementations of DuplexChannel delegate the shutdownOutput to the underlying transport, but do not take any action on the ChannelOutboundBuffer. In the event of a write failure due to the underlying transport failing and application may attempt to shutdown the output and allow the read side the transport to finish and detect the close. However this may result in an issue where writes are failed, this generates a writability change, we continue to write more data, and this may lead to another writability change, and this loop may continue. Shutting down the output should fail all pending writes and not allow any future writes to avoid this scenario. Modifications: - Implementations of DuplexChannel should null out the ChannelOutboundBuffer and fail all pending writes Result: More controlled sequencing for shutting down the output side of a channel.

Scottmitch · 2017-08-03T20:02:04Z

@normanmaurer - Comments addressed ... wdyt about #7041 (comment) ?

normanmaurer · 2017-08-04T06:29:59Z

@Scottmitch like I said please open an issue and assign to me :)
#7041 (comment)

Scottmitch · 2017-08-04T18:37:08Z

4.1 (237a4da) 4.0 (bc8ddd4)

Scottmitch added the defect label Aug 1, 2017

Scottmitch self-assigned this Aug 1, 2017

Scottmitch requested a review from normanmaurer August 1, 2017 16:27

normanmaurer requested changes Aug 1, 2017

View reviewed changes

Scottmitch force-pushed the shutdown_output branch 2 times, most recently from 1dd62da to ccd33aa Compare August 1, 2017 20:53

Scottmitch commented Aug 1, 2017

View reviewed changes

Scottmitch force-pushed the shutdown_output branch from ccd33aa to 24dfdd2 Compare August 1, 2017 20:59

Scottmitch mentioned this pull request Aug 1, 2017

EpollSocketGatheringWriteTest Intermittent Test Failure #3138

Closed

netkins reviewed Aug 2, 2017

View reviewed changes

normanmaurer reviewed Aug 3, 2017

View reviewed changes

Scottmitch force-pushed the shutdown_output branch from 24dfdd2 to 1221f66 Compare August 3, 2017 20:02

normanmaurer approved these changes Aug 4, 2017

View reviewed changes

Scottmitch closed this Aug 4, 2017

Scottmitch deleted the shutdown_output branch August 4, 2017 18:37

This was referenced Aug 4, 2017

Writing outside the event loop generates multiple writability change notifications #7053

Open

OioSocketChannel writability change notifications #7054

Closed

SocketStringEchoTest timeouts #7056

Closed

normanmaurer added this to the 4.0.51.Final milestone Aug 24, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shutting down the outbound side of the channel should not accept futu… #7041

Shutting down the outbound side of the channel should not accept futu… #7041

Scottmitch commented Aug 1, 2017

normanmaurer left a comment

normanmaurer Aug 1, 2017

Scottmitch Aug 1, 2017

normanmaurer Aug 1, 2017

normanmaurer Aug 1, 2017

Scottmitch Aug 1, 2017

Scottmitch Aug 1, 2017

normanmaurer Aug 3, 2017

Scottmitch Aug 4, 2017

Scottmitch Aug 1, 2017

normanmaurer Aug 3, 2017

Scottmitch Aug 4, 2017

Scottmitch commented Aug 1, 2017

Scottmitch commented Aug 2, 2017

netkins Aug 2, 2017

netkins Aug 2, 2017

normanmaurer Aug 3, 2017

Scottmitch commented Aug 2, 2017

Scottmitch commented Aug 3, 2017

normanmaurer Aug 3, 2017

normanmaurer Aug 3, 2017

Scottmitch commented Aug 3, 2017

normanmaurer commented Aug 4, 2017

Scottmitch commented Aug 4, 2017

Shutting down the outbound side of the channel should not accept futu… #7041

Shutting down the outbound side of the channel should not accept futu… #7041

Conversation

Scottmitch commented Aug 1, 2017

normanmaurer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Scottmitch commented Aug 1, 2017

Scottmitch commented Aug 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Scottmitch commented Aug 2, 2017

Scottmitch commented Aug 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Scottmitch commented Aug 3, 2017

normanmaurer commented Aug 4, 2017

Scottmitch commented Aug 4, 2017