You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mina.apache.org by Emmanuel Lecharny <el...@gmail.com> on 2008/09/03 08:08:06 UTC
PorotocolCodecFilter potential pbs and improvement
Hi,
while adding some Javadoc into this class, I have found that the
encoder/decoder instances creation might be not thread safe. We are
using a getDecoder0() where we have such a code :
private ProtocolDecoder getDecoder0(IoSession session) throws
Exception {
ProtocolDecoder decoder = (ProtocolDecoder) session
.getAttribute(DECODER);
if (decoder == null) {
decoder = factory.getDecoder(session);
session.setAttribute(DECODER, decoder);
}
return decoder;
}
This method is called for every messageReceived() invocation :
public void messageReceived(NextFilter nextFilter, IoSession session,
Object message) throws Exception {
if (!(message instanceof IoBuffer)) {
nextFilter.messageReceived(session, message);
return;
}
IoBuffer in = (IoBuffer) message;
ProtocolDecoder decoder = getDecoder0(session);
...
I know it's very unlikely that we receive two messages on the same
session at the same time, but this can be a possibility, AFAIK. I
suggest we synchronize this portion of the code this way :
private ProtocolDecoder getDecoder0(IoSession session) throws
Exception {
// We have to synchronize this section as we may have to create
the decoder
// here on the first invocation.
synchronized (factory) {
ProtocolDecoder decoder = (ProtocolDecoder)
session.getAttribute(DECODER);
if ( decoder == null ) {
// No existing instance ? Create it and stores it into
the session
decoder = factory.getDecoder(session);
session.setAttribute(DECODER, decoder);
}
return decoder;
}
Now, I think we can d something slightly better : we have two methods -
getDecoder0(IoSession) and getDecoder(IoSession) - doing the exact same
thing (returning the session decoder), except the fact that
getDecoder0() inject the decoder into the session, if it was not already
done. It makes me think that we might want to do taht during the
createSession() event processing, instead of doing it while processing
the first message received.
We will just have to implement the sessionCreated() method, and then we
can remove the getDecoder0() method, using the getDecoder() method
instead, without any synchronization.
thoughts ?
--
--
cordialement, regards,
Emmanuel Lécharny
www.iktek.com
directory.apache.org
Re: PorotocolCodecFilter potential pbs and improvement
Posted by Maarten Bosteels <mb...@gmail.com>.
On Sat, Sep 6, 2008 at 12:35 AM, Emmanuel Lecharny <el...@gmail.com> wrote:
> Maarten Bosteels wrote:
>>>>
>>>> I think that will be a nice improvement: simpler and more thread-safe,
>>>> don't see any downsides.
>>>>
>>>>
>>>
>>> Here is a proposal :
>>>
>>> /**
>>> * Associate a decoder and encoder instances to the newly created
>>> session.
>>> *
>>> * @param nextFilter The next filter to invoke when having processed
>>> the current
>>> * method
>>> * @param session The newly created session
>>> * @throws Exception if we can't create instances of the decoder or
>>> encoder
>>> */
>>> @Override
>>> public void sessionCreated(NextFilter nextFilter, IoSession session)
>>> throws Exception {
>>> // Creates the decoder and stores it into the newly created session
>>> ProtocolDecoder decoder = factory.getDecoder(session);
>>> session.setAttribute(DECODER, decoder);
>>>
>>> // Creates the encoder and stores it into the newly created session
>>> ProtocolEncoder encoder = factory.getEncoder(session);
>>> session.setAttribute(ENCODER, encoder);
>>>
>>> // Call the next filter
>>> nextFilter.sessionCreated(session);
>>> }
>>>
>>
>> Don't know whether it's documented somewhere, but looking at
>> ExecutorFilter code, I think it's safe to say that sessionCreated will
>> always be called BEFORE messageReceived. (That's another promise to
>> add to the list of framework guarantees, I guess.)
>>
>
> I'm not so sure...
>>
>> So your proposal looks OK to me.
>>
>
> I had a problem with the ChatClient example, as the ProtocolFilter wasn't
> added into the chain immediately, so its sessionCreated() handler was not
> called...
>
> Also I don't really know why we are storing a decoder/encoder per session,
> when usually all the sessions will use the very same encoder/decoder
> instances? If the codec is stateless, we just have to instanciate it once
> for the whole server, and resue it.
"usually" is the problem here :-) Some people prefer stateful decoders:
see http://www.nabble.com/Tutorial-on-ProtocolCodecFilter%2C-state-and-threads-to11254694.html
This option is also mentionned in the protocol-codec tutorial :
http://mina.apache.org/tutorial-on-protocolcodecfilter-for-mina-2x.html
Maarten
>
> wdyt ?
>
> --
> --
> cordialement, regards,
> Emmanuel Lécharny
> www.iktek.com
> directory.apache.org
>
>
>
Re: PorotocolCodecFilter potential pbs and improvement
Posted by Emmanuel Lecharny <el...@gmail.com>.
Maarten Bosteels wrote:
>>> I think that will be a nice improvement: simpler and more thread-safe,
>>> don't see any downsides.
>>>
>>>
>> Here is a proposal :
>>
>> /**
>> * Associate a decoder and encoder instances to the newly created session.
>> *
>> * @param nextFilter The next filter to invoke when having processed
>> the current
>> * method
>> * @param session The newly created session
>> * @throws Exception if we can't create instances of the decoder or encoder
>> */
>> @Override
>> public void sessionCreated(NextFilter nextFilter, IoSession session)
>> throws Exception {
>> // Creates the decoder and stores it into the newly created session
>> ProtocolDecoder decoder = factory.getDecoder(session);
>> session.setAttribute(DECODER, decoder);
>>
>> // Creates the encoder and stores it into the newly created session
>> ProtocolEncoder encoder = factory.getEncoder(session);
>> session.setAttribute(ENCODER, encoder);
>>
>> // Call the next filter
>> nextFilter.sessionCreated(session);
>> }
>>
>
> Don't know whether it's documented somewhere, but looking at
> ExecutorFilter code, I think it's safe to say that sessionCreated will
> always be called BEFORE messageReceived. (That's another promise to
> add to the list of framework guarantees, I guess.)
>
I'm not so sure...
> So your proposal looks OK to me.
>
I had a problem with the ChatClient example, as the ProtocolFilter
wasn't added into the chain immediately, so its sessionCreated() handler
was not called...
Also I don't really know why we are storing a decoder/encoder per
session, when usually all the sessions will use the very same
encoder/decoder instances? If the codec is stateless, we just have to
instanciate it once for the whole server, and resue it.
wdyt ?
--
--
cordialement, regards,
Emmanuel Lécharny
www.iktek.com
directory.apache.org
Re: PorotocolCodecFilter potential pbs and improvement
Posted by Emmanuel Lecharny <el...@gmail.com>.
>> That would imply some kind of synchornization on the session, I guess.
>> Something we can add.
>>
>
> In fact, that one is a guarantee we already have. At least when using
> an OrderedThreadPoolExecutor (or no executorfilter at all)
> see http://mina.apache.org/report/trunk/apidocs/org/apache/mina/filter/executor/ExecutorFilter.html
>
Maybe it should be true in all case. Is there any advantage of having
not-synchronized sessions ? (except the speedup we can have).
or another idea would be to forbid an incoming request on a session
which is currently being processed.
I don't have a clear idea about it yet, so consider my proposal much
more as a brainstorming.
>>> But why synchronize on the factory instead of on the session ?
>>>
>>>
>> Well, because it would imply we synchronize all the other part of the
>> server where we access the session, a little bit overkilling, IMHO.
>>
>
> Sorry, I don't understand what you are saying. Why would we have to
> change other parts of the code ?
>
> Synchronizing on the factory (which is shared by all sessions) creates
> a portential contention problem while IMO it's enough to synchronize
> on the session : we just have to ensure that we don't call
> factory.getDecoder(session) twice for one session.
>
Damn, you are right !
> But ok, this discussion is not really relevant because it's probably
> better to move this initialization to sessionCreated.
>
Yep, you are right too : it's irrelevant, and the proposal I made about
using the createSession() event was just about removing this painfull
synchronization.
>
>
> Don't know whether it's documented somewhere, but looking at
> ExecutorFilter code, I think it's safe to say that sessionCreated will
> always be called BEFORE messageReceived. (That's another promise to
> add to the list of framework guarantees, I guess.)
>
Yeah, I guess that it's the case. It does not makes sense to me that
some session may receive a message _before_ it has been created. *But* I
have to double check that the session is moved to a state where it can
be uses _after_ the sessionCreated event is sent and processed.
> So your proposal looks OK to me.
>
Fine. I will push the code then after having done the checks we have
talked about.
Thanks for the review and insightfull comments !
--
--
cordialement, regards,
Emmanuel Lécharny
www.iktek.com
directory.apache.org
Re: PorotocolCodecFilter potential pbs and improvement
Posted by Maarten Bosteels <mb...@gmail.com>.
On Fri, Sep 5, 2008 at 3:13 PM, Maarten Bosteels
<mb...@gmail.com> wrote:
> message from Emmanuel
>
> ---------- Forwarded message ----------
> From: Emmanuel Lécharny <el...@gmail.com>
> Date: Fri, Sep 5, 2008 at 3:07 PM
> Subject: Re: PorotocolCodecFilter potential pbs and improvement
> To: Maarten Bosteels <mb...@gmail.com>
>
>
> Hi Maarten,
>
> mor einline
>
> Maarten Bosteels wrote:
>>
>> Side note: I think it would be great if we had a list with such
>> guarantees made by MINA.
>> An example of another guarantee that should be in that list:
>> ProtocolDecoder.decoder(IoSession session, IoBuffer in,
>> ProtocolDecoderOutput out)
>> will NEVER be called simultaneously for the same IoSession
>>
>
> That would imply some kind of synchornization on the session, I guess.
> Something we can add.
In fact, that one is a guarantee we already have. At least when using
an OrderedThreadPoolExecutor (or no executorfilter at all)
see http://mina.apache.org/report/trunk/apidocs/org/apache/mina/filter/executor/ExecutorFilter.html
>>>
>>> synchronized (factory) {
>>> ProtocolDecoder decoder = (ProtocolDecoder)
>>> session.getAttribute(DECODER);
>>>
>>> if ( decoder == null ) {
>>> // No existing instance ? Create it and stores it into the
>>> session
>>> decoder = factory.getDecoder(session);
>>> session.setAttribute(DECODER, decoder);
>>> }
>>> return decoder;
>>> }
>>>
>>>
>>
>> But why synchronize on the factory instead of on the session ?
>>
>
> Well, because it would imply we synchronize all the other part of the
> server where we access the session, a little bit overkilling, IMHO.
Sorry, I don't understand what you are saying. Why would we have to
change other parts of the code ?
Synchronizing on the factory (which is shared by all sessions) creates
a portential contention problem while IMO it's enough to synchronize
on the session : we just have to ensure that we don't call
factory.getDecoder(session) twice for one session.
But ok, this discussion is not really relevant because it's probably
better to move this initialization to sessionCreated.
>>
>>
>>>
>>> Now, I think we can d something slightly better : we have two methods -
>>> getDecoder0(IoSession) and getDecoder(IoSession) - doing the exact same
>>> thing (returning the session decoder), except the fact that getDecoder0()
>>> inject the decoder into the session, if it was not already done. It makes me
>>> think that we might want to do taht during the createSession() event
>>> processing, instead of doing it while processing the first message
>>> received.
>>>
>>> We will just have to implement the sessionCreated() method, and then we can
>>> remove the getDecoder0() method, using the getDecoder() method instead,
>>> without any synchronization.
>>>
>>> thoughts ?
>>>
>>
>> I think that will be a nice improvement: simpler and more thread-safe,
>> don't see any downsides.
>>
>
> Here is a proposal :
>
> /**
> * Associate a decoder and encoder instances to the newly created session.
> *
> * @param nextFilter The next filter to invoke when having processed
> the current
> * method
> * @param session The newly created session
> * @throws Exception if we can't create instances of the decoder or encoder
> */
> @Override
> public void sessionCreated(NextFilter nextFilter, IoSession session)
> throws Exception {
> // Creates the decoder and stores it into the newly created session
> ProtocolDecoder decoder = factory.getDecoder(session);
> session.setAttribute(DECODER, decoder);
>
> // Creates the encoder and stores it into the newly created session
> ProtocolEncoder encoder = factory.getEncoder(session);
> session.setAttribute(ENCODER, encoder);
>
> // Call the next filter
> nextFilter.sessionCreated(session);
> }
Don't know whether it's documented somewhere, but looking at
ExecutorFilter code, I think it's safe to say that sessionCreated will
always be called BEFORE messageReceived. (That's another promise to
add to the list of framework guarantees, I guess.)
So your proposal looks OK to me.
Regards,
Maarten
>
> --
> --
> cordialement, regards,
> Emmanuel Lécharny
> www.nextury.com
> directory.apache.org
>
Fwd: PorotocolCodecFilter potential pbs and improvement
Posted by Maarten Bosteels <mb...@gmail.com>.
message from Emmanuel
---------- Forwarded message ----------
From: Emmanuel Lécharny <el...@gmail.com>
Date: Fri, Sep 5, 2008 at 3:07 PM
Subject: Re: PorotocolCodecFilter potential pbs and improvement
To: Maarten Bosteels <mb...@gmail.com>
Hi Maarten,
mor einline
Maarten Bosteels wrote:
>
> Side note: I think it would be great if we had a list with such
> guarantees made by MINA.
> An example of another guarantee that should be in that list:
> ProtocolDecoder.decoder(IoSession session, IoBuffer in,
> ProtocolDecoderOutput out)
> will NEVER be called simultaneously for the same IoSession
>
That would imply some kind of synchornization on the session, I guess.
Something we can add.
>>
>> synchronized (factory) {
>> ProtocolDecoder decoder = (ProtocolDecoder)
>> session.getAttribute(DECODER);
>>
>> if ( decoder == null ) {
>> // No existing instance ? Create it and stores it into the
>> session
>> decoder = factory.getDecoder(session);
>> session.setAttribute(DECODER, decoder);
>> }
>> return decoder;
>> }
>>
>>
>
> But why synchronize on the factory instead of on the session ?
>
Well, because it would imply we synchronize all the other part of the
server where we access the session, a little bit overkilling, IMHO.
>
>
>>
>> Now, I think we can d something slightly better : we have two methods -
>> getDecoder0(IoSession) and getDecoder(IoSession) - doing the exact same
>> thing (returning the session decoder), except the fact that getDecoder0()
>> inject the decoder into the session, if it was not already done. It makes me
>> think that we might want to do taht during the createSession() event
>> processing, instead of doing it while processing the first message
>> received.
>>
>> We will just have to implement the sessionCreated() method, and then we can
>> remove the getDecoder0() method, using the getDecoder() method instead,
>> without any synchronization.
>>
>> thoughts ?
>>
>
> I think that will be a nice improvement: simpler and more thread-safe,
> don't see any downsides.
>
Here is a proposal :
/**
* Associate a decoder and encoder instances to the newly created session.
*
* @param nextFilter The next filter to invoke when having processed
the current
* method
* @param session The newly created session
* @throws Exception if we can't create instances of the decoder or encoder
*/
@Override
public void sessionCreated(NextFilter nextFilter, IoSession session)
throws Exception {
// Creates the decoder and stores it into the newly created session
ProtocolDecoder decoder = factory.getDecoder(session);
session.setAttribute(DECODER, decoder);
// Creates the encoder and stores it into the newly created session
ProtocolEncoder encoder = factory.getEncoder(session);
session.setAttribute(ENCODER, encoder);
// Call the next filter
nextFilter.sessionCreated(session);
}
--
--
cordialement, regards,
Emmanuel Lécharny
www.nextury.com
directory.apache.org
Re: PorotocolCodecFilter potential pbs and improvement
Posted by Maarten Bosteels <mb...@gmail.com>.
Hello,
On Wed, Sep 3, 2008 at 8:08 AM, Emmanuel Lecharny <el...@gmail.com> wrote:
> Hi,
>
> while adding some Javadoc into this class, I have found that the
> encoder/decoder instances creation might be not thread safe. We are using a
> getDecoder0() where we have such a code :
>
> private ProtocolDecoder getDecoder0(IoSession session) throws Exception {
> ProtocolDecoder decoder = (ProtocolDecoder) session
> .getAttribute(DECODER);
> if (decoder == null) {
> decoder = factory.getDecoder(session);
> session.setAttribute(DECODER, decoder);
> }
> return decoder;
> }
>
AFAICS there is indeed a chance that we call factory.getDecoder()
twice for the same session.
Normally this shouldn't cause too much harm. But people might expect
that getDecoder is only called once per session; not sure that is
documented somewhere.
Side note: I think it would be great if we had a list with such
guarantees made by MINA.
An example of another guarantee that should be in that list:
ProtocolDecoder.decoder(IoSession session, IoBuffer in,
ProtocolDecoderOutput out)
will NEVER be called simultaneously for the same IoSession
> This method is called for every messageReceived() invocation :
>
> public void messageReceived(NextFilter nextFilter, IoSession session,
> Object message) throws Exception {
> if (!(message instanceof IoBuffer)) {
> nextFilter.messageReceived(session, message);
> return;
> }
>
> IoBuffer in = (IoBuffer) message;
> ProtocolDecoder decoder = getDecoder0(session);
> ...
>
> I know it's very unlikely that we receive two messages on the same session
> at the same time, but this can be a possibility, AFAIK. I suggest we
> synchronize this portion of the code this way :
>
>
> private ProtocolDecoder getDecoder0(IoSession session) throws Exception {
> // We have to synchronize this section as we may have to create the
> decoder
> // here on the first invocation.
> synchronized (factory) {
> ProtocolDecoder decoder = (ProtocolDecoder)
> session.getAttribute(DECODER);
>
> if ( decoder == null ) {
> // No existing instance ? Create it and stores it into the
> session
> decoder = factory.getDecoder(session);
> session.setAttribute(DECODER, decoder);
> }
> return decoder;
> }
>
But why synchronize on the factory instead of on the session ?
> Now, I think we can d something slightly better : we have two methods -
> getDecoder0(IoSession) and getDecoder(IoSession) - doing the exact same
> thing (returning the session decoder), except the fact that getDecoder0()
> inject the decoder into the session, if it was not already done. It makes me
> think that we might want to do taht during the createSession() event
> processing, instead of doing it while processing the first message
> received.
>
> We will just have to implement the sessionCreated() method, and then we can
> remove the getDecoder0() method, using the getDecoder() method instead,
> without any synchronization.
>
> thoughts ?
I think that will be a nice improvement: simpler and more thread-safe,
don't see any downsides.
Regards,
Maarten
>
> --
> --
> cordialement, regards,
> Emmanuel Lécharny
> www.iktek.com
> directory.apache.org
>
>
>