Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.0-RC1
    • Fix Version/s: 2.0.0
    • Component/s: Filter
    • Labels:
      None

      Description

      in sample code "reverser", there is code like this:
      acceptor.getFilterChain().addLast(
      "codec",
      new ProtocolCodecFilter(new TextLineCodecFactory(Charset
      .forName("UTF-8"))));

      and that means there is only one instance of TextLineCodecFactory, which holds one instance of TextLineEncoder, and TextLineDecoder has a member var named as "IoBuffer delimBuf".
      Is it thread safe to call decode() using this delimBuf?
      I think it's a bug.

        Activity

        Hide
        Emmanuel Lecharny added a comment -

        The textLineCodecfactory and the TextLineEncoder/Decoder are stateless. Thus they are thread safe.

        Show
        Emmanuel Lecharny added a comment - The textLineCodecfactory and the TextLineEncoder/Decoder are stateless. Thus they are thread safe.
        Hide
        Emmanuel Lecharny added a comment -

        Not a bug.

        Show
        Emmanuel Lecharny added a comment - Not a bug.
        Hide
        Jacklondon Chen added a comment -

        TextLineDecoder is NOT stateless with following code:

        public class TextLineDecoder implements ProtocolDecoder

        { private ByteBuffer delimBuf; }

        Show
        Jacklondon Chen added a comment - TextLineDecoder is NOT stateless with following code: public class TextLineDecoder implements ProtocolDecoder { private ByteBuffer delimBuf; }
        Hide
        Jacklondon Chen added a comment -

        this issue also exists in version 1.1.7

        Show
        Jacklondon Chen added a comment - this issue also exists in version 1.1.7
        Hide
        Emmanuel Lecharny added a comment -

        It's only used when initializing the TextLine decoder, as it stores the delimiter, something not likely to change once initialized.

        I gonna check if we can protect this initialization (I'm not sure this is done only once or if there is a potential race condition where 2 threads can set up this value).

        Thanks for your insight.

        Show
        Emmanuel Lecharny added a comment - It's only used when initializing the TextLine decoder, as it stores the delimiter, something not likely to change once initialized. I gonna check if we can protect this initialization (I'm not sure this is done only once or if there is a potential race condition where 2 threads can set up this value). Thanks for your insight.
        Hide
        Emmanuel Lecharny added a comment -

        Ok, fixed with http://svn.apache.org/viewvc?rev=936203&view=rev

        I also cleaned the code in the mean time, added the missing Javadoc

        Show
        Emmanuel Lecharny added a comment - Ok, fixed with http://svn.apache.org/viewvc?rev=936203&view=rev I also cleaned the code in the mean time, added the missing Javadoc
        Hide
        Jacklondon Chen added a comment -

        for the code:
        private void decodeNormal(Context ctx, ByteBuffer in, ProtocolDecoderOutput out)
        throws CharacterCodingException {

        while (in.hasRemaining()) {
        byte b = in.get();
        if (delimBuf.get(matchCount) == b) { ====> this should be NOT thread safe to access delimBuf when it's singleton.

        I suggest to save this delimBuf in session attribute instead of as a class member of TextLineDecoder .

        Show
        Jacklondon Chen added a comment - for the code: private void decodeNormal(Context ctx, ByteBuffer in, ProtocolDecoderOutput out) throws CharacterCodingException { while (in.hasRemaining()) { byte b = in.get(); if (delimBuf.get(matchCount) == b) { ====> this should be NOT thread safe to access delimBuf when it's singleton. I suggest to save this delimBuf in session attribute instead of as a class member of TextLineDecoder .
        Hide
        Emmanuel Lecharny added a comment -

        delimBuf is not a singleton. The way it's written now, where this field is initialized in the constructor, is 100% thread safe.

        Show
        Emmanuel Lecharny added a comment - delimBuf is not a singleton. The way it's written now, where this field is initialized in the constructor, is 100% thread safe.
        Hide
        Jacklondon Chen added a comment -

        in code of TextLineCodecFactory,
        public class TextLineCodecFactory implements ProtocolCodecFactory {
        private final TextLineDecoder decoder;
        public ProtocolDecoder getDecoder()

        { return decoder; }

        which means one TextLineCodecFactory object holds one TextLineDecoder object, and one TextLineDecoder holds one delimBuf .
        That are all singleton from code:
        acceptor.getFilterChain().addLast(
        "codec",
        new ProtocolCodecFilter(new TextLineCodecFactory(Charset
        .forName("UTF-8"))));
        Could you tell me which one is not singleton? ProtocolCodecFilter/TextLineCodecFactory/TextLineDecoder/delimBuf ?

        Show
        Jacklondon Chen added a comment - in code of TextLineCodecFactory, public class TextLineCodecFactory implements ProtocolCodecFactory { private final TextLineDecoder decoder; public ProtocolDecoder getDecoder() { return decoder; } which means one TextLineCodecFactory object holds one TextLineDecoder object, and one TextLineDecoder holds one delimBuf . That are all singleton from code: acceptor.getFilterChain().addLast( "codec", new ProtocolCodecFilter(new TextLineCodecFactory(Charset .forName("UTF-8")))); Could you tell me which one is not singleton? ProtocolCodecFilter/TextLineCodecFactory/TextLineDecoder/delimBuf ?
        Hide
        Emmanuel Lecharny added a comment -

        May be "Singleton" means something different for you than the commonly accepted definition (http://en.wikipedia.org/wiki/Singleton_pattern#Java). There is no singleton n the portion of code you mention.

        In any case, it's now thread safe.

        Show
        Emmanuel Lecharny added a comment - May be "Singleton" means something different for you than the commonly accepted definition ( http://en.wikipedia.org/wiki/Singleton_pattern#Java ). There is no singleton n the portion of code you mention. In any case, it's now thread safe.
        Hide
        Emmanuel Lecharny added a comment -

        Closing issues

        Show
        Emmanuel Lecharny added a comment - Closing issues

          People

          • Assignee:
            Unassigned
            Reporter:
            Jacklondon Chen
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development