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

AutoClose behavior may infinite loop #7153

Closed
wants to merge 1 commit into
base: 4.0
from

Conversation

Projects
None yet
2 participants
@Scottmitch
Member

Scottmitch commented Aug 26, 2017

Motivation:
If AutoClose is false and there is a IoException then AbstractChannel will not close the channel but instead just fail flushed element in the ChannelOutboundBuffer. AbstractChannel also notifies of writability changes, which may lead to an infinite loop if the peer has closed its read side of the socket because we will keep accepting more data but continuously fail because the peer isn't accepting writes.

Modifications:

  • If the transport throws on a write we should acknowledge that the output side of the channel has been shutdown and cleanup. If the channel can't accept more data because it is full, and still healthy it is not expected to throw. However if the channel is not healthy it will throw and is not expected to accept any more writes. In this case we should shutdown the output for Channels that support this feature and otherwise just close.
  • Connection-less protocols like UDP can remain the same because the channel may disconnected temporarily.
  • Make sure AbstractUnsafe#shutdownOutput is called because the shutdown on the socket may throw an exception.

Result:
More correct handling of write failure when AutoClose is false.

AutoClose behavior may infinite loop
Motivation:
If AutoClose is false and there is a IoException then AbstractChannel will not close the channel but instead just fail flushed element in the ChannelOutboundBuffer. AbstractChannel also notifies of writability changes, which may lead to an infinite loop if the peer has closed its read side of the socket because we will keep accepting more data but continuously fail because the peer isn't accepting writes.

Modifications:
- If the transport throws on a write we should acknowledge that the output side of the channel has been shutdown and cleanup. If the channel can't accept more data because it is full, and still healthy it is not expected to throw. However if the channel is not healthy it will throw and is not expected to accept any more writes. In this case we should shutdown the output for Channels that support this feature and otherwise just close.
- Connection-less protocols like UDP can remain the same because the channel may disconnected temporarily.
- Make sure AbstractUnsafe#shutdownOutput is called because the shutdown on the socket may throw an exception.

Result:
More correct handling of write failure when AutoClose is false.
@Scottmitch

This comment has been minimized.

Show comment
Hide comment
@Scottmitch

Scottmitch Aug 26, 2017

Member

I opened another PR for the back port to 4.0 just to make sure I didn't mess anything up

Member

Scottmitch commented Aug 26, 2017

I opened another PR for the back port to 4.0 just to make sure I didn't mess anything up

@Scottmitch Scottmitch requested a review from normanmaurer Aug 26, 2017

@Scottmitch Scottmitch self-assigned this Aug 26, 2017

@Scottmitch Scottmitch added the defect label Aug 26, 2017

@Scottmitch

This comment has been minimized.

Show comment
Hide comment
@Scottmitch
Member

Scottmitch commented Aug 26, 2017

4.0 (932c587)

@Scottmitch Scottmitch closed this Aug 26, 2017

@Scottmitch Scottmitch deleted the Scottmitch:shutdown_write_fail_4.0 branch Aug 26, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment