james-server-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stefano Bagnara <apa...@bago.org>
Subject Re: svn commit: r1221748 - /james/protocols/trunk/api/src/main/java/org/apache/james/protocols/api/AbstractProtocolTransport.java
Date Sat, 24 Dec 2011 13:37:11 GMT
2011/12/24 Eric Charles <eric@apache.org>:
> Maybe you can give me a hint about about the 2 user threads?

The main thread is the one that write new "reponses". It is the
protocol thread and calls the writeResponse method each time a new
reponse is available.
If the response is a FutureResponse then the whole thing swithes to a
buffered more (response queue) and a listener is added to listen for
future completion.
The thread that will receive the future completion will then work to
dequeue the buffered responses.
If it finds more FutureResponses it will start the listening again,
otherwise if the queue is clear then switches back to direct writing.

So, you have 1 thread writing responses and maybe 1 thread working to
dequeue things from the buffer. Of course the real concurrency depends
on the protocol implementation, how much they use FutureResponses
instead of simple Response and how long they wait for such responses.
They have very low concurrency. In most cases you won't have new
writes untile the future response answer, so I expect most cases we
won't have concurrency at all.

> Is there a test case that ensures the class is thread-safe (or sould I ask
> 'is it feasible to have a test for this')?

IMO it is not feasible.
Maybe you can write tests to reproduce some issue on specific
hardware, but this won't be in any way a full test suite.

> How did you came to the conclusion it was not thread-safe: pure code review,
> or exceptions/abnormal behavior in an operational deployment?

For the code from 2 versions ago we even had a test case (norman wrote
it), but mainly because it was a major issue.
For the previous version, instead, it was from pure code review: the
thread that is dequeing responses loops until the queue is empty (A),
then if it is empty and there are no more futures involved then it
re-enabled direct writing (B). If another threads tried to write a new
response while the first thread was between (A) and (B) then you end
up with an element in the queue and no one taking care to write it
(and this breaks almost any protocol I know).
The fact that (A) and (B) are near and the fact that we only rarely
have 2 threads working on that means this would probably happen very
rarely in production, but still it exists.

> I believe the beauty resides in the easy-understanding. For example, if this
> class is designed to be used by only 2 threads, it should come out for the
> reader from javadoc, method, fields... namings.
> Thx,

I usually avoid using volatile/synchronization and prefer using some
class from the wonderful java concurrent package, but I didn't find a
better solution so I moved to "something working" (or at least I hope
it works, that's why I asked for review).

Stefano

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


Mime
View raw message