Tomcat 8: Firing listeners from within a synchronized(writeLock) in onWritePossible()

[Konstantin Kolinko <kn...@gmail.com>]

Hi!

Reviewing Remy's r1575545 reminded me that I had a question on the
code of AbstractServletOutputStream.onWritePossible().

The method AbstractServletOutputStream.onWritePossible() fires several events:

1) onError(),
 that was extracted into separate method in r1575545

2) listener.onWritePossible()
 that participated in deadlock that I reported in "7.0.52 vote" thread
and was fixed in another way in r1568804

http://svn.apache.org/r1568804
http://markmail.org/message/s4u2hpisqealxflb

My question is that these events are fired while holding a
"synchronized (writeLock) {" lock.
Is holding the writeLock lock is needed here.

The "on write possible" event is processed by webapp's code. I think
that in theory the web application can delegate processing to some
other thread. That other thread will hang trying to obtain the
writeLock to perform the actual writing.


I expect that releasing this write lock to be an other way to resolve
deadlock that I reported (that was fixed by r1568804).
The r1568804 itself is OK. I am just saying that this side of that
issue can also be improved.


Best regards,
Konstantin Kolinko

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

Re: Tomcat 8: Firing listeners from within a synchronized(writeLock) in onWritePossible()

[Rémy Maucherat <re...@apache.org>]

2014-03-10 10:36 GMT+01:00 Mark Thomas <ma...@apache.org>:

> > I'm not convinced these write locks are needed, there should be no write
> > concurrency in the first place.
>
> The explanation for why the lock is required is in the comments in the
> writeInternal() method. The lock is definitely required. The issue
> described in that comment was observed when running the Autobahn
> WebSocket test suite.
>

Actually, it could also happen with NIO2, so the lock on writeInternal is
justified then.

>
> > If the consensus is to keep the lock to try to avoid the most serious
> > corruption issues, then in the onWritePossible method only the call to
> > writeInternal would need to be locked, not the rest.
>
> It does look like the lock could be narrowed but I'm not sure it can be
> quite that narrow. The lock is effectively protecting buffer from
> concurrent access (some comments to that effect would not hurt).
>
> I'll add some more comments to that class and see what can be done to
> narrow that scope of the writeLock.
>
> Ok.

Rémy

Re: Tomcat 8: Firing listeners from within a synchronized(writeLock) in onWritePossible()

[Mark Thomas <ma...@apache.org>]

On 08/03/2014 20:50, Rémy Maucherat wrote:
> 2014-03-08 21:16 GMT+01:00 Konstantin Kolinko <kn...@gmail.com>:
> 
>> My question is that these events are fired while holding a
>> "synchronized (writeLock) {" lock.
>> Is holding the writeLock lock is needed here.
>>
>> The "on write possible" event is processed by webapp's code. I think
>> that in theory the web application can delegate processing to some
>> other thread. That other thread will hang trying to obtain the
>> writeLock to perform the actual writing.
>>
> I'm not convinced these write locks are needed, there should be no write
> concurrency in the first place.

The explanation for why the lock is required is in the comments in the
writeInternal() method. The lock is definitely required. The issue
described in that comment was observed when running the Autobahn
WebSocket test suite.

> If the consensus is to keep the lock to try to avoid the most serious
> corruption issues, then in the onWritePossible method only the call to
> writeInternal would need to be locked, not the rest.

It does look like the lock could be narrowed but I'm not sure it can be
quite that narrow. The lock is effectively protecting buffer from
concurrent access (some comments to that effect would not hurt).

I'll add some more comments to that class and see what can be done to
narrow that scope of the writeLock.

Mark


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

Re: Tomcat 8: Firing listeners from within a synchronized(writeLock) in onWritePossible()

[Rémy Maucherat <re...@apache.org>]

2014-03-08 21:16 GMT+01:00 Konstantin Kolinko <kn...@gmail.com>:

> My question is that these events are fired while holding a
> "synchronized (writeLock) {" lock.
> Is holding the writeLock lock is needed here.
>
> The "on write possible" event is processed by webapp's code. I think
> that in theory the web application can delegate processing to some
> other thread. That other thread will hang trying to obtain the
> writeLock to perform the actual writing.
>
> I'm not convinced these write locks are needed, there should be no write
concurrency in the first place.

If the consensus is to keep the lock to try to avoid the most serious
corruption issues, then in the onWritePossible method only the call to
writeInternal would need to be locked, not the rest.

Rémy