Camel
  1. Camel
  2. CAMEL-5225

camel-netty can't distinguish between Sharable and Unsharable codecs

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.9.2
    • Fix Version/s: 2.9.3, 2.10.0
    • Component/s: camel-netty
    • Labels:
    • Environment:

      ALL

    • Estimated Complexity:
      Advanced

      Description

      Camel-netty uses general configuration model for referenced encoders/decoders for channel pipelines, see DefaultServerPipelineFactory. That is, create encoder/decoder objects at configuration parsing time and store them in a list, then use them when a pipeline is established. However, this will make encoder/decoder objects shared among different pipelines, that may cause data conflicts, when the encoder/decoder is not Sharable(has object status/not annotated as @Sharable), e.g. a LengthFieldBasedFrameDecoder.

      Although we can avoid the problem by totally writing a new serverpipelinefactory for our apps, several problem still remains, please see detailed description and testcase for this bug at:

      http://camel.465427.n5.nabble.com/camel-netty-bug-and-the-need-of-best-practice-for-creating-referenced-parameter-object-on-looking-up-td5627926.html

        Activity

        Hide
        edge wang added a comment -

        here is the test for showing this bug, as mentioned in the post at nabble, you can get random results each time you run the testcase when this bug exists.

        Show
        edge wang added a comment - here is the test for showing this bug, as mentioned in the post at nabble, you can get random results each time you run the testcase when this bug exists.
        Hide
        Claus Ibsen added a comment - - edited

        camel-netty have been improved for Camel 2.10.

        And if you use decoders/encoders that cannot be shared with multiple consumers, then you need to enlist 2 in the registry etc.
        This is not a bug, but how its designed. You as the end user must configure the endpoints/components correctly.

        I have added your test to the source and it passes on trunk.

        Show
        Claus Ibsen added a comment - - edited camel-netty have been improved for Camel 2.10. And if you use decoders/encoders that cannot be shared with multiple consumers, then you need to enlist 2 in the registry etc. This is not a bug, but how its designed. You as the end user must configure the endpoints/components correctly. I have added your test to the source and it passes on trunk.
        Hide
        edge wang added a comment -

        Thank you very much for your response and adding my testcase to the trunk. However, your answer makes me aware of two things:

        1.My testcase is very very misleading (so sorry for that), makes people think the problem is due to sharing decoders among consumers, but that's not the case, as I explained very clearly in the posts, it is because you share them among pipelines, not consumers.

        2.Camel-netty in version 2.10 (as well as 2.9.2) introduced a new bug, severe than the one I reported, breaks netty's pipeline model more than before. Hence even I correct the implementation of DefaultServerPipelineFactory, the testcase still breaks.

        So here I attach the correct testcase, and do some reference to show the problem I reported as well as the new bug introduced.

        sorry again for the previous misleading testcase

        Show
        edge wang added a comment - Thank you very much for your response and adding my testcase to the trunk. However, your answer makes me aware of two things: 1.My testcase is very very misleading (so sorry for that), makes people think the problem is due to sharing decoders among consumers, but that's not the case, as I explained very clearly in the posts, it is because you share them among pipelines , not consumers. 2.Camel-netty in version 2.10 (as well as 2.9.2) introduced a new bug, severe than the one I reported, breaks netty's pipeline model more than before. Hence even I correct the implementation of DefaultServerPipelineFactory, the testcase still breaks. So here I attach the correct testcase, and do some reference to show the problem I reported as well as the new bug introduced. sorry again for the previous misleading testcase
        Hide
        edge wang added a comment -

        Here is the correct testcase and the patch to pass the testing. The patch simply creates a new decoder(by hard coding that matches the testcase) each time when getpipeline is called, when you run the testcase against patched 2.9.1 version of cammel-netty, you get it passed, but not for 2.9.2 and 2.10.

        Show
        edge wang added a comment - Here is the correct testcase and the patch to pass the testing. The patch simply creates a new decoder(by hard coding that matches the testcase) each time when getpipeline is called, when you run the testcase against patched 2.9.1 version of cammel-netty, you get it passed, but not for 2.9.2 and 2.10.
        Hide
        edge wang added a comment -

        Here are some references:

        1.showing that unsharable decoders should not be shared among channels(pipelines)

        http://stackoverflow.com/questions/9254800/is-framedecoder-not-safe-in-non-single-connection-situation

        and of course the mentioned url:

        http://netty.io/docs/stable/api/org/jboss/netty/channel/ChannelHandler.Sharable.html

        2.the evidence showing the new bug

        Line 183-185 of NettyConsumer.java in version 2.9.2 ant 2.10:
        ---------------------------------------------------------------------------------------------------
        // must get the pipeline from the factory when opening a new connection
        ChannelPipeline serverPipeline = pipelineFactory.getPipeline(this);
        serverBootstrap.setPipeline(serverPipeline);
        ---------------------------------------------------------------------------------------------------

        The netty javadoc for ServerBootStrap.setPipeline:
        ---------------------------------------------------------------------------------------
        public void setPipeline(ChannelPipeline pipeline)
        Sets the default ChannelPipeline which is cloned when a new Channel is created. Bootstrap creates a new pipeline which has the same entries with the specified pipeline for a new channel.
        Calling this method also sets the pipelineFactory property to an internal ChannelPipelineFactory implementation which returns a shallow copy of the specified pipeline.

        Please note that this method is a convenience method that works only when 1) you create only one channel from this bootstrap (e.g. one-time client-side or connectionless channel) or 2) the ChannelPipelineCoverage of all handlers in the pipeline is "all". You have to use setPipelineFactory(ChannelPipelineFactory) if 1) your pipeline contains a ChannelHandler whose ChannelPipelineCoverage is "one" and 2) one or more channels are going to be created by this bootstrap (e.g. server-side channels).
        ----------------------------------------------------------------------------------------

        So you are setting the pipeline to the serverbootstrap for netty consumer (which opens server-side channels), simply breaks the second situation when "You have to use setPipelineFactory(ChannelPipelineFactory)". However, the implementation in 2.9.1 and before is correct.

        Show
        edge wang added a comment - Here are some references: 1.showing that unsharable decoders should not be shared among channels(pipelines) http://stackoverflow.com/questions/9254800/is-framedecoder-not-safe-in-non-single-connection-situation and of course the mentioned url: http://netty.io/docs/stable/api/org/jboss/netty/channel/ChannelHandler.Sharable.html 2.the evidence showing the new bug Line 183-185 of NettyConsumer.java in version 2.9.2 ant 2.10: --------------------------------------------------------------------------------------------------- // must get the pipeline from the factory when opening a new connection ChannelPipeline serverPipeline = pipelineFactory.getPipeline(this); serverBootstrap.setPipeline(serverPipeline); --------------------------------------------------------------------------------------------------- The netty javadoc for ServerBootStrap.setPipeline: --------------------------------------------------------------------------------------- public void setPipeline(ChannelPipeline pipeline) Sets the default ChannelPipeline which is cloned when a new Channel is created. Bootstrap creates a new pipeline which has the same entries with the specified pipeline for a new channel. Calling this method also sets the pipelineFactory property to an internal ChannelPipelineFactory implementation which returns a shallow copy of the specified pipeline. Please note that this method is a convenience method that works only when 1) you create only one channel from this bootstrap (e.g. one-time client-side or connectionless channel) or 2) the ChannelPipelineCoverage of all handlers in the pipeline is "all". You have to use setPipelineFactory(ChannelPipelineFactory) if 1) your pipeline contains a ChannelHandler whose ChannelPipelineCoverage is "one" and 2) one or more channels are going to be created by this bootstrap (e.g. server-side channels). ---------------------------------------------------------------------------------------- So you are setting the pipeline to the serverbootstrap for netty consumer (which opens server-side channels), simply breaks the second situation when "You have to use setPipelineFactory(ChannelPipelineFactory)". However, the implementation in 2.9.1 and before is correct.
        Hide
        Claus Ibsen added a comment -

        Thanks for reporting.

        I have refactored to use the pipeline factory, so we support stateful codecs

        Show
        Claus Ibsen added a comment - Thanks for reporting. I have refactored to use the pipeline factory, so we support stateful codecs
        Hide
        edge wang added a comment -

        Would you please adopt my revised UnsharableCodecsConflictsTest.java and run the test? I only saw the code somewhat reverted to version 2.9.1, but didn't solve the problem I reported. The problem exists as long as you use code like this(DefaultServerPipelineFactory.java):

        List<ChannelUpstreamHandler> decoders = consumer.getConfiguration().getDecoders();
        for (int x = 0; x < decoders.size(); x++)

        { channelPipeline.addLast("decoder-" + x, decoders.get(x)); }

        this configuration model causes the decoders(created and cached in consumer.getConfiguration().getDecoders()) to be shared among pinelines and then lead to conflicts on stateful codecs.

        Show
        edge wang added a comment - Would you please adopt my revised UnsharableCodecsConflictsTest.java and run the test? I only saw the code somewhat reverted to version 2.9.1, but didn't solve the problem I reported. The problem exists as long as you use code like this(DefaultServerPipelineFactory.java): List<ChannelUpstreamHandler> decoders = consumer.getConfiguration().getDecoders(); for (int x = 0; x < decoders.size(); x++) { channelPipeline.addLast("decoder-" + x, decoders.get(x)); } this configuration model causes the decoders(created and cached in consumer.getConfiguration().getDecoders()) to be shared among pinelines and then lead to conflicts on stateful codecs.
        Hide
        Claus Ibsen added a comment -

        Okay I have further improved the code to support for shareable and non-shareable encoders/decoders.

        Show
        Claus Ibsen added a comment - Okay I have further improved the code to support for shareable and non-shareable encoders/decoders.

          People

          • Assignee:
            Claus Ibsen
            Reporter:
            edge wang
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development