(Another) Invalid FileDescriptor causing infinite loop

[blucas]

Netty version: 5.0.0-Alpha2-SNAPSHOT (28/02/2015 jar)

Attention: @normanmaurer
Related issue: #3443
Event Loop: Epoll ET (issue would also occur on LT)

Hi guys,
I've found another case whereby an Invalid file descriptor exception can occur, which triggers an endless loop of epollInReady()calls.

The stack trace is exactly the same as in issue #3443 (if I could have re-opened that issue, I would have).

The fix norman supplied before helps, but doesn't solve all cases. The underlying issue is that epollInReady does not check to see if the channel is open before reading from it or before scheduling another epollInReady call.

Example:

1. Channel is active and socket contains 50KB of data
2. epollInReady is called and reads 1KB of data into a buffer and triggers pipeline.fireChannelRead
3. A handler in the pipeline closes the channel for whatever reason (e.g. SslHandler closes the channel due to handshake failure).
4. epollInReady will attempt to read more data from the socket (as it is in a do-while loop), but will throw an java.lang.IllegalStateException: invalid file descriptor as the previous iteration of the loop closed the channel
5. epollInReady will enter the catch clause (due to the IllegalStateException) and reschedule a new epollInReady event
6. The new epollInReady event will be triggered and attempt to read from the socket causing another java.lang.IllegalStateException: invalid file descriptor and the cycle will continue

I believe there are a few places that need to check if the channel is still open before attempting to read.

1. The do-while loop should exit if the channel is no longer open - this prevents attempting to make a subsequent read from a closed socket
2. The catch clause should not schedule another epollInReady if the socket is already closed
3. The Runnable task should not call epollInReady if the socket is already closed

I might have gone a bit overboard adding the check to all these places, so I'd appreciate it if someone would confirm if I am correct about this or not.

[normanmaurer]

Thanks for reporting.... The fix should be different but will fix it asap

Am 01.03.2015 um 18:26 schrieb Brendt notifications@github.com:

Netty version: 5.0.0-Alpha2-SNAPSHOT (28/02/2015 jar)

Attention: @normanmaurer
Related issue: #3443
Event Loop: Epoll ET (issue would also occur on LT)

Hi guys,
I've found another case whereby an Invalid file descriptor exception can occur, which triggers an endless loop of epollInReady()calls.

The stack trace is exactly the same as in issue #3443 (if I could have re-opened that issue, I would have).

The fix norman supplied before helps, but doesn't solve all cases. The underlying issue is that epollInReady does not check to see if the channel is open before reading from it or before scheduling another epollInReady call.

Example:

Channel is active and socket contains 50KB of data
epollInReady is called and reads 1KB of data into a buffer and triggers pipeline.fireChannelRead
A handler in the pipeline closes the channel for whatever reason (e.g. SslHandler closes the channel due to handshake failure).
epollInReady will attempt to read more data from the socket (as it is in a do-while loop), but will throw an java.lang.IllegalStateException: invalid file descriptor as the previous iteration of the loop closed the channel
epollInReady will enter the catch clause (due to the IllegalStateException) and reschedule a new epollInReady event
The new epollInReady event will be triggered and attempt to read from the socket causing another java.lang.IllegalStateException: invalid file descriptor and the cycle will continue
I believe there are a few places that need to check if the channel is still open before attempting to read.

The do-while loop should exit if the channel is no longer open - this prevents attempting to make a subsequent read from a closed socket
The catch clause should not schedule another epollInReady if the socket is already closed
The Runnable task should not call epollInReady if the socket is already closed
I might have gone a bit overboard adding the check to all these places, so I'd appreciate it if someone would confirm if I am correct about this or not.

—
Reply to this email directly or view it on GitHub.

[normanmaurer]

Fixed