MINA
  1. MINA
  2. DIRMINA-811

Exceptions in MessageDecoder.decode() cause problems for subsequent decode operations.

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.1
    • Fix Version/s: 2.0.3
    • Component/s: Core
    • Labels:
      None
    • Environment:
      Windows 7 and jdk1.6.0_18

      Description

      I am writing a client implementation for a binary protocol using a ProtocolCodecFilter with a DemuxingProtocolCodecFactory and a list of MessageDecoder's in the same way as the MINA SumUp example.
      I also use a DemuxingIoHandler to handle responses and exceptions and I am in general very pleased with how this works.

      But if an Exception is thrown from the decode()-method the problem begins.
      The Exception is passed on to an ExceptionHandler just as excpected but the DemuxingProtocolDecoder's state.currentDecoder is not cleared so a subsequent decode operation is passed directly to the previously used MessageDecoder's decode()-method without selecting a new decoder, using decodable()-calls. This is of course a problem as the next message could require a different decoder and chances are big that the decoding will throw a new Exception and we can't recover from that state.
      This differs from what happens if you return MessageDecoderResult.NOT_OK from decode() (where the state.currentDecoder is cleared) even though both events are documented as "protocol specification violations".

      In conjunction with this the IoBuffer can be "half read" when the Exception is thrown and must some how be cleared before new messages can be received and decoded.

      One approach to go around this is to do a try-catch in every decode()-method and skip the remaining bytes in the buffer and return NOT_OK in the catch-block.
      Sadly enough you can not BOTH return NOT_OK and re-throw the Exception. So the nice ExceptionHandler-functionality in MINA goes to waste.
      Also it feels wrong to be forced to do try-catch in every decode()-method for Exceptions that make the current message unreadable, could not the framework handle that and give subsequent messages a chance to be properly decoded?
      (Exceptions during decoding that you ARE prepared for and CAN handle to rescue the current message is a different story, the specific decode()-method can catch those and handle them in a proper way.)

      My humble suggestion is to let the DemuxingProtocolDecoder in it's doDecode() catch Exceptions, reset the current decoder and buffer and re-throw the Exceptions. But I'm no expert of MINA internals.

      I am made aware that the IoBuffer can contain more than one message and that clearing it could loose a valid second message.
      But if the decode()-method chooses not to handle the Exception, my guess is that there is no way of knowing where the first message ends and the second begins outside that MessageDecoder anyway.

        Activity

        Emmanuel Lecharny made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Assignee Emmanuel Lecharny [ elecharny ]
        Resolution Fixed [ 1 ]
        Show
        Emmanuel Lecharny added a comment - Fixed with http://svn.apache.org/viewvc?rev=1091209&view=rev
        Hide
        Emmanuel Lecharny added a comment -

        Sure, I'll keep the original exception then.

        Show
        Emmanuel Lecharny added a comment - Sure, I'll keep the original exception then.
        Hide
        Björn Thelberg added a comment -

        Oh, sorry! Didn't know about the vote at all!

        Actually I would prefer re-throwing the same exception, so that the appropriate exception handler is choosen just like before.
        That makes it backwards compatible for those who depend on a special exception handler being used for a specific exception type
        and rely on information in the original exception object.
        Also, in my opinion, there is a risk of loosing information if the original exception is not preserved.

        What do you think of that?

        Great if you could include this fix!

        Show
        Björn Thelberg added a comment - Oh, sorry! Didn't know about the vote at all! Actually I would prefer re-throwing the same exception, so that the appropriate exception handler is choosen just like before. That makes it backwards compatible for those who depend on a special exception handler being used for a specific exception type and rely on information in the original exception object. Also, in my opinion, there is a risk of loosing information if the original exception is not preserved. What do you think of that? Great if you could include this fix!
        Hide
        Emmanuel Lecharny added a comment -

        Damn it... Just when the vote for MINA-2.0.3 has been launched

        I have added the suggested patch (slightly modified) :

        try

        { MessageDecoderResult result = state.currentDecoder.decode(session, in, out); ... }

        catch (Exception e)

        { state.currentDecoder = null; throw new IllegalStateException( "Unexpected decode result (see your decode()): " + e.getMessage() ); }

        Can you tell me if the IllegalStateException is ok ?

        I may stop the vote, and integrate this patch (plus another one), and restart a vote asap (ie, in the next couple of days.

        Thanks a lot for the contribution !

        Show
        Emmanuel Lecharny added a comment - Damn it... Just when the vote for MINA-2.0.3 has been launched I have added the suggested patch (slightly modified) : try { MessageDecoderResult result = state.currentDecoder.decode(session, in, out); ... } catch (Exception e) { state.currentDecoder = null; throw new IllegalStateException( "Unexpected decode result (see your decode()): " + e.getMessage() ); } Can you tell me if the IllegalStateException is ok ? I may stop the vote, and integrate this patch (plus another one), and restart a vote asap (ie, in the next couple of days. Thanks a lot for the contribution !
        Hide
        Björn Thelberg added a comment -

        We have been using the following fix for the DemuxingProtocolDecoder for some months of heavy testing now and it has worked fine.
        We are no longer stuck in the same decoder after an exception and the exception handling works fine.

        DemuxingProtocolDecoder:175

        // — Start of fix for DIRMINA-811
        MessageDecoderResult result;
        try

        { result = state.currentDecoder.decode(session, in, out); }

        catch (Exception e)

        { state.currentDecoder = null; throw e; }

        // — End of fix for DIRMINA-811

        Show
        Björn Thelberg added a comment - We have been using the following fix for the DemuxingProtocolDecoder for some months of heavy testing now and it has worked fine. We are no longer stuck in the same decoder after an exception and the exception handling works fine. DemuxingProtocolDecoder:175 // — Start of fix for DIRMINA-811 — MessageDecoderResult result; try { result = state.currentDecoder.decode(session, in, out); } catch (Exception e) { state.currentDecoder = null; throw e; } // — End of fix for DIRMINA-811 —
        Emmanuel Lecharny made changes -
        Fix Version/s 2.0.3 [ 12315510 ]
        Fix Version/s 2.0.2 [ 12315473 ]
        Emmanuel Lecharny made changes -
        Field Original Value New Value
        Fix Version/s 2.0.2 [ 12315473 ]
        Björn Thelberg created issue -

          People

          • Assignee:
            Emmanuel Lecharny
            Reporter:
            Björn Thelberg
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development