grizzly
  1. grizzly
  2. GRIZZLY-109

ThreadAttachment.process() unconditionally removes application selection key attachment

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: framework
    • Labels:
      None
    • Environment:

      Operating System: All
      Platform: All

    • Issuezilla Id:
      109

      Description

      Current code for ThreadAttachment.process():

      public void process(SelectionKey selectionKey)

      { ((WorkerThread) Thread.currentThread()).attach( (ThreadAttachment) this); selectionKey.attach(null); }

      As you can see it unconditionally removes any application-specific selection
      key attachments. This is the primary reason that makes impossible usage of
      custom selection key attachments and ParserProtocolFilter together (the rest
      of the framework works with the custom attachments just fine provided they are
      subclasses of SelectionKeyAttachment).

        Activity

        Hide
        jfarcand added a comment -

        Thanks for the report! Re-assign to Alexey

        Show
        jfarcand added a comment - Thanks for the report! Re-assign to Alexey
        Hide
        oleksiys added a comment -

        Hello.

        IMHO current implementation is safest one, as ThreadAttachments always worked like that. If now we will
        remove selectionKey.attach(null); we can break someone?
        If ThreadAttachment subclasses want some different behavior - they can override proccess method.

        What do you think?

        Show
        oleksiys added a comment - Hello. IMHO current implementation is safest one, as ThreadAttachments always worked like that. If now we will remove selectionKey.attach(null); we can break someone? If ThreadAttachment subclasses want some different behavior - they can override proccess method. What do you think?
        Hide
        dets added a comment -

        Well, situation with ThreadAttachment and selection key attachments is not
        that simple. I'll try to explain my point of view.

        First of all, I think that it is a rather reasonable assumption that selection
        key attachments usually can be used to store some connection state information
        FROM THE START and TILL THE END of the connection. Second, it seems that the
        most appropriate places to track connection start and attach something to
        selection key is SelectorHandler.onAcceptInterest() and for the end of
        connection - SelectionKeyHandler.cancel().

        Now, if you attach a new ThreadAttachment in onAcceptInterest then during
        request processing ThreadAttachment.process will be called and it'll be
        attached to the WorkerThread. And it'll set WorkerThread's byteBuffer to
        null That is, unless you create and set some byteBuffer in
        ThreadAttachment in onAcceptInterest handler, in this case it'll replace
        WorkerThread's byteBuffer. BTW, AFAIK program can't automatically find out
        what should be the default size and type of byteBuffer. I believe that this is
        a very ugly solution and managing of byteBuffer should be done completely by
        WorkerThread, it's its internal implementation detail.

        And it is not any better in SelectionKeyHandler.cancel() - the same
        ThreadAttachment.process() will remove your attachment from the key and so
        you'll not be able to access your attachment at all.

        As you say one can subclass ThreadAttachment and override process() - this
        will help in the case of cancel handler but the problem with creation of
        byteBuffer will remain. Apart from this if changing of
        ThreadAttachment.process can break some parts of the framework (I don't think
        so) then custom subclass will also break it. And usually there should be no
        reason at all to subclass ThreadAttachment because it has
        setAttribute/getAttribute methods that already allow to attach any user data
        to ThreadAttachment (and so to selection key).

        Another problem is that ThreadAttachment behavior contradicts the rest of the
        framework (the rest respects custom attachments set using
        SelectionKeyAttachmentWrapper) and available documentation (i.e. "SelectionKey
        attachment utility class. This class could be used as wrapper for custom
        SelectionKey attachments, which are not subclasses of SelectionKeyAttachment
        class." - in practice it can be used only if ParserProtocolFilter with its
        ThreadAttachment is not used).

        Conclusion: I strongly believe that this is a bug because it makes custom
        selection key attachments hardly usable if at all. ThreadAttachment.process()
        should be changed to not attach null. WorkerThreadImpl.attach should be
        changed to be a bit more intelligent to a) respect existing key attachments;
        b) doesn't overwrite thread's byteBuffer with null byteBuffer (and other null
        buffers?) from ThreadAttachment.

        BTW, it is not related to this bug, just a thought: IMHO in Grizzly it'll be
        acceptable and sufficient to support attachments of ThreadAttachment ONLY.
        Because one can attach any user data to selection key using
        ThreadAttachment.setAttribute. And because attachment of anything else de
        facto doesn't work even now. And judging by your comment "always worked like
        that" never worked before I mean in general it doesn't work now, there
        are some case when it can work (i.e. if you'll not use ParserProtocolFilter).

        Show
        dets added a comment - Well, situation with ThreadAttachment and selection key attachments is not that simple. I'll try to explain my point of view. First of all, I think that it is a rather reasonable assumption that selection key attachments usually can be used to store some connection state information FROM THE START and TILL THE END of the connection. Second, it seems that the most appropriate places to track connection start and attach something to selection key is SelectorHandler.onAcceptInterest() and for the end of connection - SelectionKeyHandler.cancel(). Now, if you attach a new ThreadAttachment in onAcceptInterest then during request processing ThreadAttachment.process will be called and it'll be attached to the WorkerThread. And it'll set WorkerThread's byteBuffer to null That is, unless you create and set some byteBuffer in ThreadAttachment in onAcceptInterest handler, in this case it'll replace WorkerThread's byteBuffer. BTW, AFAIK program can't automatically find out what should be the default size and type of byteBuffer. I believe that this is a very ugly solution and managing of byteBuffer should be done completely by WorkerThread, it's its internal implementation detail. And it is not any better in SelectionKeyHandler.cancel() - the same ThreadAttachment.process() will remove your attachment from the key and so you'll not be able to access your attachment at all. As you say one can subclass ThreadAttachment and override process() - this will help in the case of cancel handler but the problem with creation of byteBuffer will remain. Apart from this if changing of ThreadAttachment.process can break some parts of the framework (I don't think so) then custom subclass will also break it. And usually there should be no reason at all to subclass ThreadAttachment because it has setAttribute/getAttribute methods that already allow to attach any user data to ThreadAttachment (and so to selection key). Another problem is that ThreadAttachment behavior contradicts the rest of the framework (the rest respects custom attachments set using SelectionKeyAttachmentWrapper) and available documentation (i.e. "SelectionKey attachment utility class. This class could be used as wrapper for custom SelectionKey attachments, which are not subclasses of SelectionKeyAttachment class." - in practice it can be used only if ParserProtocolFilter with its ThreadAttachment is not used). Conclusion: I strongly believe that this is a bug because it makes custom selection key attachments hardly usable if at all. ThreadAttachment.process() should be changed to not attach null. WorkerThreadImpl.attach should be changed to be a bit more intelligent to a) respect existing key attachments; b) doesn't overwrite thread's byteBuffer with null byteBuffer (and other null buffers?) from ThreadAttachment. BTW, it is not related to this bug, just a thought: IMHO in Grizzly it'll be acceptable and sufficient to support attachments of ThreadAttachment ONLY. Because one can attach any user data to selection key using ThreadAttachment.setAttribute. And because attachment of anything else de facto doesn't work even now. And judging by your comment "always worked like that" never worked before I mean in general it doesn't work now, there are some case when it can work (i.e. if you'll not use ParserProtocolFilter).
        Hide
        oleksiys added a comment -

        Hello,

        thank you very much for the feedback!!!
        I agree with your proposal.

        > And judging by your comment "always worked like
        > that" never worked before

        I'll try to express other words. Now and before ThreadAttachment worked as "one time" state storage.
        Once used (processed) it always was removed from SelectionKey's attachment. And next time if state
        should be stored - SelectionKey.attach had to be called again.
        Not saying that this is good solution!
        Applying change you propose can break some existing custom code, which relies on that.

        Anyway think the propose is good!

        Thanks.

        Show
        oleksiys added a comment - Hello, thank you very much for the feedback!!! I agree with your proposal. > And judging by your comment "always worked like > that" never worked before I'll try to express other words. Now and before ThreadAttachment worked as "one time" state storage. Once used (processed) it always was removed from SelectionKey's attachment. And next time if state should be stored - SelectionKey.attach had to be called again. Not saying that this is good solution! Applying change you propose can break some existing custom code, which relies on that. Anyway think the propose is good! Thanks.
        Hide
        jfarcand added a comment -

        Fix from Alexey will be commited 04/07.

        Show
        jfarcand added a comment - Fix from Alexey will be commited 04/07.
        Hide
        oleksiys added a comment -

        fixed

        Show
        oleksiys added a comment - fixed

          People

          • Assignee:
            oleksiys
            Reporter:
            dets
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: