Bug 61003 - Secure Websocket client hides Exception on error, and throws an IllegalStateException instead
Summary: Secure Websocket client hides Exception on error, and throws an IllegalStateE...
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 8
Classification: Unclassified
Component: WebSocket (show other bugs)
Version: 8.5.11
Hardware: PC Linux
: P2 normal (vote)
Target Milestone: ----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-04-18 15:50 UTC by Alexandre de Champeaux
Modified: 2017-05-04 11:34 UTC (History)
0 users



Attachments
The stack trace (1.29 KB, text/plain)
2017-04-18 15:50 UTC, Alexandre de Champeaux
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Alexandre de Champeaux 2017-04-18 15:50:03 UTC
Created attachment 34924 [details]
The stack trace

We encounter the following exception (full stack attached) instead of a proper exception, such as a time out:

java.lang.IllegalStateException: Concurrent write operations are not permitted
	at org.apache.tomcat.websocket.AsyncChannelWrapperSecure.write(AsyncChannelWrapperSecure.java:116)

AsyncChannelWrapperSecure javadoc interestingly states "This needs a lot more testing before it can be considered robust.", and indeed this class seems to be the cause of the issue. More details about what I suspect is going on:


If you take a closer look at WsRemoteEndpointImplBase.sendMessageBlock line 313, you will find this section:

         for (MessagePart mp : messageParts) {
            writeMessagePart(mp);
            if (!bsh.getSendResult().isOK()) {
                messagePartInProgress.release();
                Throwable t = bsh.getSendResult().getException();
 // Bug alert!! Here we will write again, but may not have reset the writing flag of AsyncChannelWrapperSecure to false 
                wsSession.doClose(new CloseReason(CloseCodes.GOING_AWAY, t.getMessage()),
                        new CloseReason(CloseCodes.CLOSED_ABNORMALLY, t.getMessage()));
                throw new IOException (t);
            }

which is problematic: The writeMessagePart method will eventually call doWrite of WsRemoteEndpointImplClient, which will simply return in case of exceptions:

  try {
                channel.write(byteBuffer).get(timeout, TimeUnit.MILLISECONDS);
            } catch (InterruptedException | ExecutionException | TimeoutException e) {
                handler.onResult(new SendResult(e));
                return;
            }

So here, when the get fails, the  WriteTask of AsyncChannelWrapperSecure, will not have finished, and so will not have unset its write flag, meaning that the wsSession.doClose call done in WsRemoteEndpointImplBase.sendMessageBlock will fail to write. 
Moreover, the write flag of the WriteTask is not reset in the finally block, meaning that any other exception thrown by the write task will cause the completion of the future, but will not allow to write in the ws either, causing another failure.

This will mean that it will fail to throw the actual cause of the exception stored in the SendResult.


Side Note: the ReadTask seems to suffer of the same disease, not sure if it is immune to it.
Comment 1 Mark Thomas 2017-04-19 20:34:07 UTC
If you add  writing.set(false); /  reading.set(false); to the catch blocks of the WriteTask and ReadTask respectively, does that improve things?
Comment 2 Mark Thomas 2017-05-02 10:03:09 UTC
Without any feedback, I intend to go ahead and apply the proposed change from comment #1 and then resolve this issue.
Comment 3 Violeta Georgieva 2017-05-04 11:34:09 UTC
Hi,

Thanks for the report.
The fix was committed in:
- trunk for 9.0.0.M21 onwards
- 8.5.x for 8.5.15 onwards
- 8.0.x for 8.0.44 onwards
- 7.0.x for 7.0.78 onwards

Regards,
Violeta