Uploaded image for project: 'MINA'
  1. MINA
  2. DIRMINA-1013

Threading model is supressed by ProtocolCodecFilter

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.9
    • Fix Version/s: 2.0.10
    • Component/s: Core, Filter
    • Labels:
      None
    • Environment:
      Windows 7 x32
      Java(TM) SE Runtime Environment (build 1.8.0_45-b14)
      Java HotSpot(TM) Client VM (build 25.45-b02, mixed mode, sharing)
    • Flags:
      Important

      Description

      ProtocolCodecFilter.messageReceived uses a semaphore to protect the following critical section:

                      lock.acquire();
                      // Call the decoder with the read bytes
                      decoder.decode(session, in, decoderOut);
                      // Finish decoding if no exception was thrown.
                      decoderOut.flush(nextFilter, session);
                      ...
      

      in such fragment of code:

              // Loop until we don't have anymore byte in the buffer,
              // or until the decoder throws an unrecoverable exception or
              // can't decoder a message, because there are not enough
              // data in the buffer
              while (in.hasRemaining()) {
                  int oldPos = in.position();
                  try {
                      lock.acquire();
                      // Call the decoder with the read bytes
                      decoder.decode(session, in, decoderOut);
                      // Finish decoding if no exception was thrown.
                      decoderOut.flush(nextFilter, session);
                  } catch (Exception e) {
                      ProtocolDecoderException pde;
                      if (e instanceof ProtocolDecoderException) {
                          pde = (ProtocolDecoderException) e;
                      } else {
                          pde = new ProtocolDecoderException(e);
                      }
                      if (pde.getHexdump() == null) {
                          // Generate a message hex dump
                          int curPos = in.position();
                          in.position(oldPos);
                          pde.setHexdump(in.getHexDump());
                          in.position(curPos);
                      }
                      // Fire the exceptionCaught event.
                      decoderOut.flush(nextFilter, session);
                      nextFilter.exceptionCaught(session, pde);
                      // Retry only if the type of the caught exception is
                      // recoverable and the buffer position has changed.
                      // We check buffer position additionally to prevent an
                      // infinite loop.
                      if (!(e instanceof RecoverableProtocolDecoderException) || (in.position() == oldPos)) {
                          break;
                      }
                  } finally {
                      lock.release();
                  }
              }
      

      Using of semaphore

      public class ProtocolCodecFilter extends IoFilterAdapter {
      ...
          private final Semaphore lock = new Semaphore(1, true);
      

      pushs other threads to wait while one of them is decoding. In MINA 2.0.7 there was a synchronized block at the same place, but on other point - decoderOut, wich is created per ioSession. Thus it was a stripped lock.

        Issue Links

          Activity

          Show
          elecharny Emmanuel Lecharny added a comment - Should be fixed with http://git-wip-us.apache.org/repos/asf/mina/commit/849e22af
          Hide
          elecharny Emmanuel Lecharny added a comment -

          Attached to the other issue.

          Show
          elecharny Emmanuel Lecharny added a comment - Attached to the other issue.
          Hide
          elecharny Emmanuel Lecharny added a comment -

          See last comment on DIRMINA-934

          Show
          elecharny Emmanuel Lecharny added a comment - See last comment on DIRMINA-934
          Hide
          elecharny Emmanuel Lecharny added a comment -

          My bad ! I was thinking 'session', not 'decoder'.

          There are a few thing to take care of :

          • a decoder might be stateful, thus keeping a state for further decoding. In this case, we must keep such decoder instance associated to a session, not to a thread (or a IoProcessor).
          • if the decoder is stateless, then we don't need any lock, as we wont have any possible interaction with any other thread.

          Actually, I don't see why we do protect the decoding at all in the ProtocolCodecFilter, it should be the codec responsability to protect itself against concurrent actions.

          Show
          elecharny Emmanuel Lecharny added a comment - My bad ! I was thinking 'session', not 'decoder'. There are a few thing to take care of : a decoder might be stateful, thus keeping a state for further decoding. In this case, we must keep such decoder instance associated to a session, not to a thread (or a IoProcessor). if the decoder is stateless, then we don't need any lock, as we wont have any possible interaction with any other thread. Actually, I don't see why we do protect the decoding at all in the ProtocolCodecFilter , it should be the codec responsability to protect itself against concurrent actions.
          Hide
          mgainullin Marat Gainullin added a comment -

          If lock will be associated with decoder instance, nothing will change (it will not lead to parallel decoding as well). Parallel decoding/encoding is critical for our TSA server. In MINA 2.0.7 lock was associated with decoderOut, not decoder, thus leading to parallel decoding.

          Show
          mgainullin Marat Gainullin added a comment - If lock will be associated with decoder instance, nothing will change (it will not lead to parallel decoding as well). Parallel decoding/encoding is critical for our TSA server. In MINA 2.0.7 lock was associated with decoderOut, not decoder, thus leading to parallel decoding.
          Hide
          elecharny Emmanuel Lecharny added a comment -

          This is a SERIOUS issue you just found ! The lock should be associated to the decoder instance, not to the ProtocolCodecFilter.

          We will investigate this pb urgently, and probably cut a release asap.

          Many thanks !

          Show
          elecharny Emmanuel Lecharny added a comment - This is a SERIOUS issue you just found ! The lock should be associated to the decoder instance, not to the ProtocolCodecFilter . We will investigate this pb urgently, and probably cut a release asap. Many thanks !

            People

            • Assignee:
              Unassigned
              Reporter:
              mgainullin Marat Gainullin
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development