james-server-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Eric Charles <e...@apache.org>
Subject Re: svn commit: r1221748 - /james/protocols/trunk/api/src/main/java/org/apache/james/protocols/api/AbstractProtocolTransport.java
Date Tue, 27 Dec 2011 05:05:51 GMT
Sorry Stefano :)

Thx Norman, I finally found the AbstractProtocolTransportTest (test 
source folder was not in my build path.

Eric


On 24/12/11 16:28, Norman Maurer wrote:
> Hi Eric,
>
> Its Stefano not Stephano ;)
>
> Comments inside...
>
> Am Samstag, 24. Dezember 2011 schrieb Eric Charles<eric@apache.org>:
>> Hi Stephano, Thx for the inputs. I'm fine to have volatile fields and to
> use them is synchronized blocks when needed, and out-of synchronized blocks
> when possible.
>>
>> I still have to find the few hours to better understand the usage and
> context.
>>
>> Maybe you can give me a hint about about the 2 user threads?
>>
>> 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')?
>>
>
> Check Out the AbstractProtocolTransportTest..
>
>> How did you came to the conclusion it was not thread-safe: pure code
> review, or exceptions/abnormal behavior in an operational deployment?
>
> I noticed that the Responses were written out of order at a paid Project .
> That was why I was starting to investigate..
>>
>> 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.
>
> As this is more a internal thing I would Not mention it.
>>
>> Thx,
>>
>> Eric
>
> Bye,
> Norman
>
>>
>>
>> On 24/12/11 12:19, Stefano Bagnara wrote:
>>
>> 2011/12/24 Eric Charles<eric@apache.org>:
>>
>> Hi Stephano,
>>
>> Opening the discussion to learn more :)
>>
>> - Why are you considering that 2 threads is a criteria to use standard
>> synchronization rather than some atomic fields.
>>
>> It is a criteria to not use the ConcurrentLinkedQueue that is a
>> structure thought to handle many concurrent threads and is overkill
>> for 2 threads.
>>
>> - I can understand you replace a concurrent by a non-concurrent queue.
>> However, you now have a blocking queue. Is there an impact due to this
>> blocking aspect?
>>
>> I think the answer is no. That's why I did that.
>> Remember we have 2 threads and they do 2 different things, they simply
>> block each other when they add or remove from the queue.
>>
>> - You defined isAsync as volatile and sometimes encapsulate access to
>> isAsync in a synchronized block, sometime not. Why using 2 different
>> thread-safety strategies in this class?
>>
>> Because some times it needed sincronization, other times I felt it was
>> not needed (the access to the volatile doesn't need synchronization. I
>> just synchronize to ensure that the change to the list happens
>> together with the change in the volatile var).
>> If you can find a better solution you're welcome to provide one. It
>> took a couple of hours to reach a working solution.
>>
>> The previous one was not thread safe at this line:
>> ------
>> if (listenerAdded == false) {
>>    write.set(false);
>> -----
>> It could happen another thread already added a new item to the queue
>> but skipped to process it because write was true. So we ended up with
>> an item in the queue never written.
>>
>> I don't like too much my solution and I felt it a bit hackish, but
>> that was my best solution for my limited time, so if you can provide a
>> more elegant solution while still being thread safe, I'm more than
>> happy :-)
>>
>> Stefano
>>
>>
>> Thx,
>>
>> Eric
>>
>>
>>
>> On 21/12/11 15:47, bago@apache.org wrote:
>>
>> Author: bago
>> Date: Wed Dec 21 14:47:25 2011
>> New Revision: 1221748
>>
>> URL: http://svn.apache.org/viewvc?rev=1221748&view=rev
>> Log:
>> An attempt to refactor AbstractProtocolTransport to be thread safe. I
>> moved back to standard synchronization as we only have max 2 threads
>> competing for the queue so it doesn't make sense to use a non blocking
>> queue. Norman, please overview, and feel free to revert if you don't like
>> the solution (i thought it was better to simply commit instead of opening
> a
>> JIRA to show you this).
>>
>> Modified:
>>
>>
> james/protocols/trunk/api/src/main/java/org/apache/james/protocols/api/AbstractProtocolTransport.java
>>
>> Modified:
>>
> james/protocols/trunk/api/src/main/java/org/apache/james/protocols/api/AbstractProtocolTransport.java
>> URL:
>>
> http://svn.apache.org/viewvc/james/protocols/trunk/api/src/main/java/org/apache/james/protocols/api/AbstractProtocolTransport.java?rev=1221748&r1=1221747&r2=1221748&view=diff
>>
>>
> ==============================================================================
>> ---
>>
> james/protocols/trunk/api/src/main/java/org/apache/james/protocols/api/AbstractProtocolTransport.java
>> (original)
>> +++
>>
> james/protocols/trunk/api/src/main/java/org/apache/james/protocols/api/AbstractProtocolTransport.java
>> Wed Dec 21 14:47:25 2011
>> @@ -22,9 +22,8 @@ package org.apache.james.protocols.api;
>>   import java.io.InputStream;
>>   import java.io.UnsupportedEncodingException;
>>   import java.util.List;
>> -import java.util.concurrent.ConcurrentLinkedQueue;
>> -import java.util.concurrent.atomic.AtomicBool
>

-- 
eric | http://about.echarles.net | @echarles

---------------------------------------------------------------------
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