You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mina.apache.org by Haviv <ha...@gmail.com> on 2007/03/26 14:48:09 UTC
CumulativeProtocolDecoder BUG ?
Hi,
I am working on MINA 1.0.1 , using a custom ProtocolDecoder which extends
the CumulativeProtocolDecoder.
I am also using an ExecutorFilter in front of the ProtocolCodecFilter -
meaning that my ProtocolDecoder runs
multi threaded.
Recently under Heavy load I have encountered some strange errors - my
received messages got corrupted.
After looking at the CumulativeProtocolDecoder - I was wondering why there
isn't any synchronization block
on CumulativeProtocolDecoder.decode method which can prevent this kind of
behavior ?
I am meaning something like :
public void decode(IoSession session, ByteBuffer in,ProtocolDecoderOutput
out) throws Exception {
synchronized (session) {
...
...
...
}
}
After all there is a call to the flip method(from
CumulativeProtocolDecoder.decode) which reset the position,
while other threads may be doing the doDecode stuff - using the buffer
position.
Thanks,
Haviv
--
View this message in context: http://www.nabble.com/CumulativeProtocolDecoder-BUG---tf3466819.html#a9672586
Sent from the mina dev mailing list archive at Nabble.com.
Re: CumulativeProtocolDecoder BUG ?
Posted by Marcin Waldowski <M....@sulechow.net>.
Unfortunately something weird happened with your replay for Trustin
answer. In your message source I see that all lines is preceded by '> '
char. So I afaraid that community can overlook it.
Regards, Marcin
Haviv wrote:
>>
>> There is synch parts on the ExecutorFilter.fireEvent :
>> synchronized( buf.eventQueue )
>> {
>> buf.eventQueue.add( event );
>> if( buf.processingCompleted )
>> {
>> buf.processingCompleted = false;
>> if ( logger.isDebugEnabled() ) {
>> logger.debug( "Launching thread for " +
>> session.getRemoteAddress() );
>> }
>>
>> executor.execute( new ProcessEventsRunnable( buf ) );
>> }
>> }
>> But this method just puts the Runnable on a Q, and the
>> CumulativeProtocolDecoder runs on a different thread.
>>
>> There is another synch block in ProcessEventsRunnable:
>> public void run()
>> {
>> while( true )
>> {
>> Event event;
>>
>> synchronized( buffer.eventQueue )
>> {
>> if( buffer.eventQueue.isEmpty() )
>> {
>> buffer.processingCompleted = true;
>> break;
>> }
>>
>> event = ( Event ) buffer.eventQueue.remove( 0 );
>> }
>>
>> processEvent( event.getNextFilter(), buffer.session,
>> event.getType(), event.getData() );
>> }
>>
>> if ( logger.isDebugEnabled() ) {
>> logger.debug( "Exiting since queue is empty for " +
>> buffer.session.getRemoteAddress() );
>> }
>> }
>> But as you can see the processEvent is actually outside of the block.
>>
>>
>>
>>
>
>
Re: CumulativeProtocolDecoder BUG ?
Posted by Haviv <ha...@gmail.com>.
Trustin Lee wrote:
>
> Hi Haviv,
>
> On 3/26/07, Haviv <ha...@gmail.com> wrote:
>> I am working on MINA 1.0.1 , using a custom ProtocolDecoder which extends
>> the CumulativeProtocolDecoder.
>> I am also using an ExecutorFilter in front of the ProtocolCodecFilter -
>> meaning that my ProtocolDecoder runs
>> multi threaded.
>> Recently under Heavy load I have encountered some strange errors - my
>> received messages got corrupted.
>> After looking at the CumulativeProtocolDecoder - I was wondering why
>> there
>> isn't any synchronization block
>> on CumulativeProtocolDecoder.decode method which can prevent this kind of
>> behavior ?
>
> CumulativeProtocolDecoder doesn't need to synchronize anything because
> ExecutorFilter guarantees that only one event per session is processed
> at the same time.
>
> There is synch parts on the ExecutorFilter.fireEvent :
> synchronized( buf.eventQueue )
> {
> buf.eventQueue.add( event );
> if( buf.processingCompleted )
> {
> buf.processingCompleted = false;
> if ( logger.isDebugEnabled() ) {
> logger.debug( "Launching thread for " +
> session.getRemoteAddress() );
> }
>
> executor.execute( new ProcessEventsRunnable( buf ) );
> }
> }
> But this method just puts the Runnable on a Q, and the
> CumulativeProtocolDecoder runs on a different thread.
>
> There is another synch block in ProcessEventsRunnable:
> public void run()
> {
> while( true )
> {
> Event event;
>
> synchronized( buffer.eventQueue )
> {
> if( buffer.eventQueue.isEmpty() )
> {
> buffer.processingCompleted = true;
> break;
> }
>
> event = ( Event ) buffer.eventQueue.remove( 0 );
> }
>
> processEvent( event.getNextFilter(), buffer.session,
> event.getType(), event.getData() );
> }
>
> if ( logger.isDebugEnabled() ) {
> logger.debug( "Exiting since queue is empty for " +
> buffer.session.getRemoteAddress() );
> }
> }
> But as you can see the processEvent is actually outside of the block.
>
> Did wrapping the code with the synchronized block fix your problem?
> Could you provide us that reproduces the problem so we can make sure
> if it is really a bug of CumulativeProtocolDecoder before blindly
> applying your fix?
>
> Applying the synch part on the CumulativeProtocolDecoder actually solved
> the problem.
> Regarding the code -I think that any Decoder which extends the
> CumulativeProtocolDecoder should tackle this problem, tell me if you still
> need it.
>
> Trustin
> --
> what we call human nature is actually human habit
> --
> http://gleamynode.net/
> --
> PGP Key ID: 0x0255ECA6
>
>
--
View this message in context: http://www.nabble.com/CumulativeProtocolDecoder-BUG---tf3466819.html#a9688245
Sent from the mina dev mailing list archive at Nabble.com.
Re: CumulativeProtocolDecoder BUG ?
Posted by Maarten Bosteels <mb...@gmail.com>.
Hello Marcin and Trustin,
On 3/29/07, Marcin Waldowski <M....@sulechow.net> wrote:
> Trustin Lee wrote:
> > ExecutorFilter ensures that two threads don't handle the same session.
> > So I think the scenario you gave won't happen. But, will we still
> > have a visibility problem with getDecoderLock() method in this case?
> > I thought both synchronized HashMap (for 1.0) and ConcurrentHashMap
> > (for 1.1+) takes care of the visibility problem. WDYT?
> >
> > Trustin
> Hello
>
> I agree with Trustin and I think that we will not have a visiblility
> problem with DECODER_LOCK attribute, bacause thread local memory will be
> flushed to main memory at the end of synchronized block around
> decoder.decode( session, in, decoderOut );
If Trustin is 100% certian that no two threads can execute the
getDecoderLock() method simultaneously for the same IoSession than
there is no problem:
they will both use the same lockObject and memory writes of one thread
will indeed be visible to the other thread.
In that case, I would mention this fact in the javadoc of the
getDecoderLock() function. Another option is to synchronize on the
IoSession in the getDecoderLock function (as I posted before).
Would this degrade performance noticeably, I don't think so ?
However, if they can execute the getDecoderLock() method
simultaneously for the same IoSession, then it is possible that they
use different lock objects.
In that case the Java Memory Model does not guarantee that writes of
one thread are visible by the other, it is absolutely essential that
they lock on the same monitor.
(it is very well explained in chapter 16 of Java Concurrency in Practice)
Maarten
>
> But I don't feel like an expert of java concurrecy. I based only on
> following article in subject of visibility:
> http://www.javaworld.com/jw-02-2001/jw-0209-double.html
>
> Regards, Marcin
>
Re: CumulativeProtocolDecoder BUG ?
Posted by Marcin Waldowski <M....@sulechow.net>.
Trustin Lee wrote:
> ExecutorFilter ensures that two threads don't handle the same session.
> So I think the scenario you gave won't happen. But, will we still
> have a visibility problem with getDecoderLock() method in this case?
> I thought both synchronized HashMap (for 1.0) and ConcurrentHashMap
> (for 1.1+) takes care of the visibility problem. WDYT?
>
> Trustin
Hello
I agree with Trustin and I think that we will not have a visiblility
problem with DECODER_LOCK attribute, bacause thread local memory will be
flushed to main memory at the end of synchronized block around
decoder.decode( session, in, decoderOut );
But I don't feel like an expert of java concurrecy. I based only on
following article in subject of visibility:
http://www.javaworld.com/jw-02-2001/jw-0209-double.html
Regards, Marcin
Re: CumulativeProtocolDecoder BUG ?
Posted by Marcin Waldowski <M....@sulechow.net>.
Haviv wrote:
> Hi,
> The ProcessEventsRunnable also doesnt process event in sequence:
> because the processEvent method call is outside the synch part :
>
>
> public void run()
> {
> while( true )
> {
> Event event;
>
> synchronized( buffer.eventQueue )
> {
> if( buffer.eventQueue.isEmpty() )
> {
> buffer.processingCompleted = true;
> break;
> }
>
> event = ( Event ) buffer.eventQueue.remove( 0 );
> }
>
> processEvent( event.getNextFilter(), buffer.session,
> event.getType(), event.getData() );
> }
>
> if ( logger.isDebugEnabled() ) {
> logger.debug( "Exiting since queue is empty for " +
> buffer.session.getRemoteAddress() );
> }
> }
>
>
Hi Haviv.
Please look inside synchronized block in ExcutorFilter fireEvent method
and please analyse for what is "if( buf.processingCompleted )" instruction.
Also look at ProcessEventsRunnable.run() and ExcutorFilter.fireEvent()
methods and analyse where buf.processingCompleted is changed and why.
Regards, Marcin
Re: CumulativeProtocolDecoder BUG ?
Posted by Haviv <ha...@gmail.com>.
Hi,
The ProcessEventsRunnable also doesnt process event in sequence:
because the processEvent method call is outside the synch part :
public void run()
{
while( true )
{
Event event;
synchronized( buffer.eventQueue )
{
if( buffer.eventQueue.isEmpty() )
{
buffer.processingCompleted = true;
break;
}
event = ( Event ) buffer.eventQueue.remove( 0 );
}
processEvent( event.getNextFilter(), buffer.session,
event.getType(), event.getData() );
}
if ( logger.isDebugEnabled() ) {
logger.debug( "Exiting since queue is empty for " +
buffer.session.getRemoteAddress() );
}
}
Marcin Waldowski-2 wrote:
>
> Haviv wrote:
>> Hi Trustin,
>>
>> Where in the code you see that "ExecutorFilter ensures that two threads
>> don't handle the same session".
>> You have said that before and I didn't understand it.
>> Looking at the ExecutorFilter(version 1.0.1) fireEvent method :
>> private void fireEvent(...)
>> {
>> Event event = new Event( type, nextFilter, data );
>> SessionBuffer buf = SessionBuffer.getSessionBuffer( session );
>>
>> synchronized( buf.eventQueue )
>> {
>> buf.eventQueue.add( event );
>> if( buf.processingCompleted )
>> {
>> buf.processingCompleted = false;
>> if ( logger.isDebugEnabled() ) {
>> logger.debug( "Launching thread for " +
>> session.getRemoteAddress() );
>> }
>>
>> executor.execute( new ProcessEventsRunnable( buf ) );
>> }
>> }
>> }
>>
>> There is a synch part but it synchronizes the executor.execute
>> method(which
>> just adds the ProcessEventsRunnable to its workQueue) and not the
>> execution
>> of the ProcessEventsRunnable itself.
>> Am I missing something ?
>>
>>
>>
>>
>
> Hi Haviv.
>
> Look at ProcessEventsRunnable class. It process event in sequence. Look
> at ExecutorFilter fireEvent() method. It ensures that only one
> ProcessEventRunnable instance can be executed by executor for one session.
>
> Did you applay patches from http://tinyurl.com/2gldgx ? Did you check if
> this solves your issue?
>
> Regards, Marcin
>
>
>
--
View this message in context: http://www.nabble.com/CumulativeProtocolDecoder-BUG---tf3466819.html#a9728766
Sent from the mina dev mailing list archive at Nabble.com.
Re: CumulativeProtocolDecoder BUG ?
Posted by Marcin Waldowski <M....@sulechow.net>.
Haviv wrote:
> Hi Trustin,
>
> Where in the code you see that "ExecutorFilter ensures that two threads
> don't handle the same session".
> You have said that before and I didn't understand it.
> Looking at the ExecutorFilter(version 1.0.1) fireEvent method :
> private void fireEvent(...)
> {
> Event event = new Event( type, nextFilter, data );
> SessionBuffer buf = SessionBuffer.getSessionBuffer( session );
>
> synchronized( buf.eventQueue )
> {
> buf.eventQueue.add( event );
> if( buf.processingCompleted )
> {
> buf.processingCompleted = false;
> if ( logger.isDebugEnabled() ) {
> logger.debug( "Launching thread for " +
> session.getRemoteAddress() );
> }
>
> executor.execute( new ProcessEventsRunnable( buf ) );
> }
> }
> }
>
> There is a synch part but it synchronizes the executor.execute method(which
> just adds the ProcessEventsRunnable to its workQueue) and not the execution
> of the ProcessEventsRunnable itself.
> Am I missing something ?
>
>
>
>
Hi Haviv.
Look at ProcessEventsRunnable class. It process event in sequence. Look
at ExecutorFilter fireEvent() method. It ensures that only one
ProcessEventRunnable instance can be executed by executor for one session.
Did you applay patches from http://tinyurl.com/2gldgx ? Did you check if
this solves your issue?
Regards, Marcin
Re: CumulativeProtocolDecoder BUG ?
Posted by Haviv <ha...@gmail.com>.
Sorry , me former message looked strange, here it is again:
Hi Trustin,
Where in the code you see that "ExecutorFilter ensures that two threads
don't handle the same session".
You have said that before and I didn't understand it.
Looking at the ExecutorFilter(version 1.0.1) fireEvent method :
private void fireEvent( ... )
{
Event event = new Event( type, nextFilter, data );
SessionBuffer buf = SessionBuffer.getSessionBuffer( session );
synchronized( buf.eventQueue )
{
buf.eventQueue.add( event );
if( buf.processingCompleted )
{
buf.processingCompleted = false;
if ( logger.isDebugEnabled() ) {
logger.debug( "Launching thread for " +
session.getRemoteAddress() );
}
executor.execute( new ProcessEventsRunnable( buf ) );
}
}
}
There is a synch part but it synchronizes the executor.execute method
(which just adds the ProcessEventsRunnable to its workQueue)
and not the execution of the ProcessEventsRunnable itself.
Am I missing something ?
Haviv wrote:
>
>
> Hi Trustin,
>
> Where in the code you see that "ExecutorFilter ensures that two threads
> don't handle the same session".
> You have said that before and I didn't understand it.
> Looking at the ExecutorFilter(version 1.0.1) fireEvent method :
> private void fireEvent(...)
> {
> Event event = new Event( type, nextFilter, data );
> SessionBuffer buf = SessionBuffer.getSessionBuffer( session );
>
> synchronized( buf.eventQueue )
> {
> buf.eventQueue.add( event );
> if( buf.processingCompleted )
> {
> buf.processingCompleted = false;
> if ( logger.isDebugEnabled() ) {
> logger.debug( "Launching thread for " +
> session.getRemoteAddress() );
> }
>
> executor.execute( new ProcessEventsRunnable( buf ) );
> }
> }
> }
>
> There is a synch part but it synchronizes the executor.execute
> method(which just adds the ProcessEventsRunnable to its workQueue) and not
> the execution of the ProcessEventsRunnable itself.
> Am I missing something ?
>
>
>
> Trustin Lee wrote:
>>
>> Hi Maarten,
>>
>> On 3/29/07, Maarten Bosteels <mb...@gmail.com> wrote:
>>> On 3/28/07, Maarten Bosteels <mb...@gmail.com> wrote:
>>> > Trustin,
>>> >
>>> > I think there is theoretically still a chance that two threads
>>> > synchronize on different lock object for the same session:
>>> >
>>> > + private Object getDecoderLock( IoSession session )
>>> > + {
>>> > + Object lock = session.getAttribute( DECODER_LOCK );
>>> > + if( lock == null )
>>> > + {
>>> > + lock = new Object();
>>> > + session.setAttribute( DECODER_LOCK, lock );
>>> > + }
>>> > +
>>> > + return lock;
>>> > + }
>>> >
>>> > thread A calls session.getAttribute( DECODER_LOCK ) and gets null
>>> > thread B calls session.getAttribute( DECODER_LOCK ) and gets null
>>> > thread A creates a new lock object lockA and stores it in the session
>>> > thread B creates a new lock object lockB and stores it in the session
>>> > thread A locks on lockA and enters decoder.decode
>>> > thread B locks on lockB and also enters decoder.decode
>>> >
>>> > I guess you can't get around locking on the IoSession instance itself
>>> ?
>>
>> ExecutorFilter ensures that two threads don't handle the same session.
>> So I think the scenario you gave won't happen. But, will we still
>> have a visibility problem with getDecoderLock() method in this case?
>> I thought both synchronized HashMap (for 1.0) and ConcurrentHashMap
>> (for 1.1+) takes care of the visibility problem. WDYT?
>>
>> Trustin
>> --
>> what we call human nature is actually human habit
>> --
>> http://gleamynode.net/
>> --
>> PGP Key ID: 0x0255ECA6
>>
>>
>
>
--
View this message in context: http://www.nabble.com/CumulativeProtocolDecoder-BUG---tf3466819.html#a9728362
Sent from the mina dev mailing list archive at Nabble.com.
Re: CumulativeProtocolDecoder BUG ?
Posted by Haviv <ha...@gmail.com>.
Hi Trustin,
Where in the code you see that "ExecutorFilter ensures that two threads
don't handle the same session".
You have said that before and I didn't understand it.
Looking at the ExecutorFilter(version 1.0.1) fireEvent method :
private void fireEvent(...)
{
Event event = new Event( type, nextFilter, data );
SessionBuffer buf = SessionBuffer.getSessionBuffer( session );
synchronized( buf.eventQueue )
{
buf.eventQueue.add( event );
if( buf.processingCompleted )
{
buf.processingCompleted = false;
if ( logger.isDebugEnabled() ) {
logger.debug( "Launching thread for " +
session.getRemoteAddress() );
}
executor.execute( new ProcessEventsRunnable( buf ) );
}
}
}
There is a synch part but it synchronizes the executor.execute method(which
just adds the ProcessEventsRunnable to its workQueue) and not the execution
of the ProcessEventsRunnable itself.
Am I missing something ?
Trustin Lee wrote:
>
> Hi Maarten,
>
> On 3/29/07, Maarten Bosteels wrote:
>> On 3/28/07, Maarten Bosteels wrote:
>> > Trustin,
>> >
>> > I think there is theoretically still a chance that two threads
>> > synchronize on different lock object for the same session:
>> >
>> > + private Object getDecoderLock( IoSession session )
>> > + {
>> > + Object lock = session.getAttribute( DECODER_LOCK );
>> > + if( lock == null )
>> > + {
>> > + lock = new Object();
>> > + session.setAttribute( DECODER_LOCK, lock );
>> > + }
>> > +
>> > + return lock;
>> > + }
>> >
>> > thread A calls session.getAttribute( DECODER_LOCK ) and gets null
>> > thread B calls session.getAttribute( DECODER_LOCK ) and gets null
>> > thread A creates a new lock object lockA and stores it in the session
>> > thread B creates a new lock object lockB and stores it in the session
>> > thread A locks on lockA and enters decoder.decode
>> > thread B locks on lockB and also enters decoder.decode
>> >
>> > I guess you can't get around locking on the IoSession instance itself ?
>
> ExecutorFilter ensures that two threads don't handle the same session.
> So I think the scenario you gave won't happen. But, will we still
> have a visibility problem with getDecoderLock() method in this case?
> I thought both synchronized HashMap (for 1.0) and ConcurrentHashMap
> (for 1.1+) takes care of the visibility problem. WDYT?
>
> Trustin
> --
> what we call human nature is actually human habit
> --
> http://gleamynode.net/
> --
> PGP Key ID: 0x0255ECA6
>
>
--
View this message in context: http://www.nabble.com/CumulativeProtocolDecoder-BUG---tf3466819.html#a9728303
Sent from the mina dev mailing list archive at Nabble.com.
Re: CumulativeProtocolDecoder BUG ?
Posted by Trustin Lee <tr...@gmail.com>.
Hi Maarten,
On 3/29/07, Maarten Bosteels <mb...@gmail.com> wrote:
> On 3/28/07, Maarten Bosteels <mb...@gmail.com> wrote:
> > Trustin,
> >
> > I think there is theoretically still a chance that two threads
> > synchronize on different lock object for the same session:
> >
> > + private Object getDecoderLock( IoSession session )
> > + {
> > + Object lock = session.getAttribute( DECODER_LOCK );
> > + if( lock == null )
> > + {
> > + lock = new Object();
> > + session.setAttribute( DECODER_LOCK, lock );
> > + }
> > +
> > + return lock;
> > + }
> >
> > thread A calls session.getAttribute( DECODER_LOCK ) and gets null
> > thread B calls session.getAttribute( DECODER_LOCK ) and gets null
> > thread A creates a new lock object lockA and stores it in the session
> > thread B creates a new lock object lockB and stores it in the session
> > thread A locks on lockA and enters decoder.decode
> > thread B locks on lockB and also enters decoder.decode
> >
> > I guess you can't get around locking on the IoSession instance itself ?
ExecutorFilter ensures that two threads don't handle the same session.
So I think the scenario you gave won't happen. But, will we still
have a visibility problem with getDecoderLock() method in this case?
I thought both synchronized HashMap (for 1.0) and ConcurrentHashMap
(for 1.1+) takes care of the visibility problem. WDYT?
Trustin
--
what we call human nature is actually human habit
--
http://gleamynode.net/
--
PGP Key ID: 0x0255ECA6
Re: CumulativeProtocolDecoder BUG ?
Posted by Maarten Bosteels <mb...@gmail.com>.
On 3/28/07, Maarten Bosteels <mb...@gmail.com> wrote:
> Trustin,
>
> I think there is theoretically still a chance that two threads
> synchronize on different lock object for the same session:
>
> + private Object getDecoderLock( IoSession session )
> + {
> + Object lock = session.getAttribute( DECODER_LOCK );
> + if( lock == null )
> + {
> + lock = new Object();
> + session.setAttribute( DECODER_LOCK, lock );
> + }
> +
> + return lock;
> + }
>
> thread A calls session.getAttribute( DECODER_LOCK ) and gets null
> thread B calls session.getAttribute( DECODER_LOCK ) and gets null
> thread A creates a new lock object lockA and stores it in the session
> thread B creates a new lock object lockB and stores it in the session
> thread A locks on lockA and enters decoder.decode
> thread B locks on lockB and also enters decoder.decode
>
> I guess you can't get around locking on the IoSession instance itself ?
private Object getDecoderLock( IoSession session )
{
synchronized (session}
{
Object lock = session.getAttribute( DECODER_LOCK );
if( lock == null )
{
lock = new Object();
session.setAttribute( DECODER_LOCK, lock );
}
return lock;
}
}
> Maarten
>
> On 3/28/07, Maarten Bosteels <mb...@gmail.com> wrote:
> > Steven,
> >
> > Have a look at the diff that Trustin posted: http://tinyurl.com/2ar8ky
> >
> > ProtocolCodecFilter stores a lock object in the session and locks on it before calling
> > decoder.decode()
> >
> > As I see it, this means that no two threads can call decoder.decode for the same session simultaneously.
> >
> > Maarten
> >
> >
> >
> > On 3/28/07, Steven E. Harris <se...@panix.com> wrote:
> > > Trustin Lee < trustin@gmail.com> writes:
> > >
> > > > By doing this, all decoders will be free from the visibility
> > > > problem.
> > >
> > > I missed the earlier part of this discussion. Does this mean that
> > > ProtocolDecoder.decode() calls will be synchronized to a particular
> > > decoder? That is, a given decoder instance will only be called on by
> > > one thread at a time?
> > >
> > > I thought we had discussed this earlier and confirmed that it was
> > > intentional that a decoder could be called on from multiple threads
> > > simultaneously.
> > >
> > > --
> > > Steven E. Harris
> > >
> >
> >
>
Re: CumulativeProtocolDecoder BUG ?
Posted by "Steven E. Harris" <se...@panix.com>.
Maarten Bosteels <mb...@gmail.com> writes:
> I think there is theoretically still a chance that two threads
> synchronize on different lock object for the same session:
This is a classic "check-then-act" problem. If we don't control the
underlying map used by IoSession.(get|set)Attribute(), preventing
approaches like the looping ConcurrentHashMap example in "Java
Concurrency in Practice"¹, we must lock at a higher level.
Footnotes:
¹ http://www.javaconcurrencyinpractice.com/listings/Memoizer.java
--
Steven E. Harris
Re: CumulativeProtocolDecoder BUG ?
Posted by Maarten Bosteels <mb...@gmail.com>.
Trustin,
I think there is theoretically still a chance that two threads
synchronize on different lock object for the same session:
+ private Object getDecoderLock( IoSession session )
+ {
+ Object lock = session.getAttribute( DECODER_LOCK );
+ if( lock == null )
+ {
+ lock = new Object();
+ session.setAttribute( DECODER_LOCK, lock );
+ }
+
+ return lock;
+ }
thread A calls session.getAttribute( DECODER_LOCK ) and gets null
thread B calls session.getAttribute( DECODER_LOCK ) and gets null
thread A creates a new lock object lockA and stores it in the session
thread B creates a new lock object lockB and stores it in the session
thread A locks on lockA and enters decoder.decode
thread B locks on lockB and also enters decoder.decode
I guess you can't get around locking on the IoSession instance itself ?
Maarten
On 3/28/07, Maarten Bosteels <mb...@gmail.com> wrote:
> Steven,
>
> Have a look at the diff that Trustin posted: http://tinyurl.com/2ar8ky
>
> ProtocolCodecFilter stores a lock object in the session and locks on it before calling
> decoder.decode()
>
> As I see it, this means that no two threads can call decoder.decode for the same session simultaneously.
>
> Maarten
>
>
>
> On 3/28/07, Steven E. Harris <se...@panix.com> wrote:
> > Trustin Lee < trustin@gmail.com> writes:
> >
> > > By doing this, all decoders will be free from the visibility
> > > problem.
> >
> > I missed the earlier part of this discussion. Does this mean that
> > ProtocolDecoder.decode() calls will be synchronized to a particular
> > decoder? That is, a given decoder instance will only be called on by
> > one thread at a time?
> >
> > I thought we had discussed this earlier and confirmed that it was
> > intentional that a decoder could be called on from multiple threads
> > simultaneously.
> >
> > --
> > Steven E. Harris
> >
>
>
Re: CumulativeProtocolDecoder BUG ?
Posted by Maarten Bosteels <mb...@gmail.com>.
Steven,
Have a look at the diff that Trustin posted: http://tinyurl.com/2ar8ky
ProtocolCodecFilter stores a lock object in the session and locks on it
before calling
decoder.decode()
As I see it, this means that no two threads can call decoder.decode for the
same session simultaneously.
Maarten
On 3/28/07, Steven E. Harris <se...@panix.com> wrote:
>
> Trustin Lee <tr...@gmail.com> writes:
>
> > By doing this, all decoders will be free from the visibility
> > problem.
>
> I missed the earlier part of this discussion. Does this mean that
> ProtocolDecoder.decode() calls will be synchronized to a particular
> decoder? That is, a given decoder instance will only be called on by
> one thread at a time?
>
> I thought we had discussed this earlier and confirmed that it was
> intentional that a decoder could be called on from multiple threads
> simultaneously.
>
> --
> Steven E. Harris
>
Re: CumulativeProtocolDecoder BUG ?
Posted by "Steven E. Harris" <se...@panix.com>.
Trustin Lee <tr...@gmail.com> writes:
> By doing this, all decoders will be free from the visibility
> problem.
I missed the earlier part of this discussion. Does this mean that
ProtocolDecoder.decode() calls will be synchronized to a particular
decoder? That is, a given decoder instance will only be called on by
one thread at a time?
I thought we had discussed this earlier and confirmed that it was
intentional that a decoder could be called on from multiple threads
simultaneously.
--
Steven E. Harris
Re: CumulativeProtocolDecoder BUG ?
Posted by Trustin Lee <tr...@gmail.com>.
On 3/28/07, Trustin Lee <tr...@gmail.com> wrote:
> Hi Maarten,
>
> On 3/27/07, Maarten Bosteels <mb...@gmail.com> wrote:
> > Trustin,
> >
> > As someone pointed out in another thread there could still be a
> > visibility problem, when multiple threads access the same objects,
> > even when they don't do it concurrently.
> >
> > I am not an expert on concurrency, I just remember that visibility is
> > very subtle.
> > I would have to re-read the chapter on visibility in "Java
> > Concurrencty in Practice"
> > to be sure if there is a problem in this case. I don't think there is, though.
> > I will try to have a look tonight.
>
> I thought there was no visibility problem in CumulativeProtocolDecoder
> because it had been protected by the synchronized block in
> ProtocolCodecFilter. Let me check it our again. :)
It seems like I removed the synchronized block. :)
To avoid performance degradation which might be caused by using
decoder instance as a lock, I chose to create a lock object and store
it as a session attribute:
http://tinyurl.com/2ar8ky
By doing this, all decoders will be free from the visibility problem.
The change has been applied to all three branches.
Thanks,
Trustin
--
what we call human nature is actually human habit
--
http://gleamynode.net/
--
PGP Key ID: 0x0255ECA6
Re: CumulativeProtocolDecoder BUG ?
Posted by Trustin Lee <tr...@gmail.com>.
Hi Maarten,
On 3/27/07, Maarten Bosteels <mb...@gmail.com> wrote:
> Trustin,
>
> As someone pointed out in another thread there could still be a
> visibility problem, when multiple threads access the same objects,
> even when they don't do it concurrently.
>
> I am not an expert on concurrency, I just remember that visibility is
> very subtle.
> I would have to re-read the chapter on visibility in "Java
> Concurrencty in Practice"
> to be sure if there is a problem in this case. I don't think there is, though.
> I will try to have a look tonight.
I thought there was no visibility problem in CumulativeProtocolDecoder
because it had been protected by the synchronized block in
ProtocolCodecFilter. Let me check it our again. :)
Thanks,
Trustin
--
what we call human nature is actually human habit
--
http://gleamynode.net/
--
PGP Key ID: 0x0255ECA6
Re: CumulativeProtocolDecoder BUG ?
Posted by Maarten Bosteels <mb...@gmail.com>.
Trustin,
As someone pointed out in another thread there could still be a
visibility problem, when multiple threads access the same objects,
even when they don't do it concurrently.
I am not an expert on concurrency, I just remember that visibility is
very subtle.
I would have to re-read the chapter on visibility in "Java
Concurrencty in Practice"
to be sure if there is a problem in this case. I don't think there is, though.
I will try to have a look tonight.
http://safari.java.net/0321349601/ch03
Maarten
On 3/27/07, Trustin Lee <tr...@gmail.com> wrote:
> Hi Haviv,
>
> On 3/26/07, Haviv <ha...@gmail.com> wrote:
> > I am working on MINA 1.0.1 , using a custom ProtocolDecoder which extends
> > the CumulativeProtocolDecoder.
> > I am also using an ExecutorFilter in front of the ProtocolCodecFilter -
> > meaning that my ProtocolDecoder runs
> > multi threaded.
> > Recently under Heavy load I have encountered some strange errors - my
> > received messages got corrupted.
> > After looking at the CumulativeProtocolDecoder - I was wondering why there
> > isn't any synchronization block
> > on CumulativeProtocolDecoder.decode method which can prevent this kind of
> > behavior ?
>
> CumulativeProtocolDecoder doesn't need to synchronize anything because
> ExecutorFilter guarantees that only one event per session is processed
> at the same time.
>
> Did wrapping the code with the synchronized block fix your problem?
> Could you provide us that reproduces the problem so we can make sure
> if it is really a bug of CumulativeProtocolDecoder before blindly
> applying your fix?
>
> Trustin
> --
> what we call human nature is actually human habit
> --
> http://gleamynode.net/
> --
> PGP Key ID: 0x0255ECA6
>
Re: CumulativeProtocolDecoder BUG ?
Posted by Trustin Lee <tr...@gmail.com>.
Hi Haviv,
On 3/26/07, Haviv <ha...@gmail.com> wrote:
> I am working on MINA 1.0.1 , using a custom ProtocolDecoder which extends
> the CumulativeProtocolDecoder.
> I am also using an ExecutorFilter in front of the ProtocolCodecFilter -
> meaning that my ProtocolDecoder runs
> multi threaded.
> Recently under Heavy load I have encountered some strange errors - my
> received messages got corrupted.
> After looking at the CumulativeProtocolDecoder - I was wondering why there
> isn't any synchronization block
> on CumulativeProtocolDecoder.decode method which can prevent this kind of
> behavior ?
CumulativeProtocolDecoder doesn't need to synchronize anything because
ExecutorFilter guarantees that only one event per session is processed
at the same time.
Did wrapping the code with the synchronized block fix your problem?
Could you provide us that reproduces the problem so we can make sure
if it is really a bug of CumulativeProtocolDecoder before blindly
applying your fix?
Trustin
--
what we call human nature is actually human habit
--
http://gleamynode.net/
--
PGP Key ID: 0x0255ECA6