On Fri, Sep 5, 2008 at 3:13 PM, Maarten Bosteels <[EMAIL PROTECTED]> wrote: > message from Emmanuel > > ---------- Forwarded message ---------- > From: Emmanuel Lécharny <[EMAIL PROTECTED]> > Date: Fri, Sep 5, 2008 at 3:07 PM > Subject: Re: PorotocolCodecFilter potential pbs and improvement > To: Maarten Bosteels <[EMAIL PROTECTED]> > > > 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 >
