2014-11-22 0:28 GMT+03:00 Konstantin Kolinko <knst.koli...@gmail.com>:
> 2014-11-21 17:57 GMT+03:00 Rémy Maucherat <r...@apache.org>:
>> Hi,
>>
>> I noticed read notifications for websockets are synced (connectionReadLock
>> in WsFrameBase)
>
> Correction: I suppose that you meant connectionReadLock in WsFrameServer
>
> (That is the only place where such name is used in o.a.tomcat.websocket
> classes)
>
>> although the only way it is called is through the read
>> notification of the upgraded stream. Which means I don't see how any
>> concurrency can occur.
>>
>> However, writes are not synced (the equivalent sync would be in
>> WsRemoteEndpointImplServer.onWritePossible) although there I believe there
>> can be some concurrency. Similarly, the buffers field and the individual
>> buffers could be at risk but it seems less likely.
>>
>> Did I miss something ?
>>
Just my thoughts looking at the current code of Tomcat 9:
1) At what point in time can the next onDataAvailable() call happen?
>From Servlet Spec (3.1 chap.3.7 "Non Blocking IO") it sounds as if as
soon as isReady() obtained result of "false" from some underlying API
the next call can happen, even before the isReady() method returns.
There are two implementations of ServletInputStream.isReady()
a) CoyoteInputStream.isReady() -> o.a.c.connector.InputBuffer.isReady().
The latter calls "coyoteRequest.action(ActionCode.NB_READ_INTEREST,
null);" before returning the "false" result, which makes it possible
to fire the next onDataAvailable() event.
b) UpgradeServletInputStream.isReady()
I think this one has a bug: it has to return the same value that it
obtained from underlying method (socketWrapper.isReady()). Instead of
that it stores the value into a volatile field and reads it back,
having a race condition.
2) At the same time, I see that there are synchronizations
synchronized (socket) {
synchronized (ka.getWriteThreadLock()) {
in NioEndpoint$SocketProcessor.run()
It may be that those can prevent the next onDataAvailable() call
arriving earlier than the current onDataAvailable() call completes,
but I am not sure of it.
3) Note that fields updated in WsFrameServer.onDataAvailable() are not
volatile, such as writePos, declared in the base class.
In WsRemoteEndpointImplServer.onWritePossible all explicitly accessed
fields are either volatile or final.
Synchronization makes writePos to be properly published between calls.
Nevertheless, with the current implementation I think it is possible
to remove synchronization as isOpen() accesses a volatile field and
that provides some memory barrier. On unsuccessful reads the field
is not modified. On successful reads isOpen() is called both before
an iteration and just after it.
The above is a bit fragile and I am concerned with abnormal aborts of
the loop after successful read, e.g. if an exception is thrown from
processInputBuffer(), as it will skip the isOpen() call after the
loop. If those aborts are not fatal then I do not see what will cause
writePos to be published after update and there might be problems.
This assumes that race bug in UpgradeServletInputStream.isReady() that
I mentioned in 1)b) is fixed.
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
