Apache AWF
  1. Apache AWF
  2. AWF-153

Dynamic creation of RequestHandlers

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Labels:
      None

      Description

      The outcome after a discussion with github.com/inferno-:
      (In the current design) it's volatile to have fields in user defined RequestHandlers in conjunction with asynchronous calls. (intermediate request (to same RH) could change the state of the RH)

      We should investigate the performance impact for dynamic creation of RH.
      Proposed solution:
      1, Map<String, RequestHandler> => Map<String, Class<RequestHandler>>
      2, Application.getHandler should create the appropriate RequestHandler using reflection.
      3, UT / benchmark

      1. deft-142_clone.patch
        254 kB
        Johnathan Meehan

        Activity

        Hide
        Johnathan Meehan added a comment -

        With an initial implementation in line with this:

        Original

        • 14480.65 /sec (mean)
        • 15229.27 /sec (mean)
        • 14367.18 /sec (mean)
        • 14283.03 /sec (mean)

        Reflection

        • 7405.82 /sec (mean)
        • 7487.22 /sec (mean)
        • 7449.57 /sec (mean)
        • 7464.47 /sec (mean)

        Oh, dear. Given the nature of the object, would we perhaps consider clone?

        Cloned

        • 14947.91 /sec (mean)
        • 16111.44 /sec (mean)
        • 14321.85 /sec (mean)
        • 14236.77 /sec (mean)
        • 14437.61 /sec (mean)
        Show
        Johnathan Meehan added a comment - With an initial implementation in line with this: Original 14480.65 /sec (mean) 15229.27 /sec (mean) 14367.18 /sec (mean) 14283.03 /sec (mean) Reflection 7405.82 /sec (mean) 7487.22 /sec (mean) 7449.57 /sec (mean) 7464.47 /sec (mean) Oh, dear. Given the nature of the object, would we perhaps consider clone? Cloned 14947.91 /sec (mean) 16111.44 /sec (mean) 14321.85 /sec (mean) 14236.77 /sec (mean) 14437.61 /sec (mean)
        Hide
        Roger Schildmeijer added a comment -

        Oh sweet lord. Even worse than I could imagine. Reflection is slow. QED.

        Regarding clone: I guess that means that we will have the clone method abstract in the RequestHandler class (to make it "easier" for third parties)? Like:
        protected abstract Object clone() throws CloneNotSupportedException;

        One different approach is to let the concrete RequestHandlers be, and use the fact that HttpRequest is unique for each request and providoe something similar to HttpSessions. Thoughts? (pros: less impact to existing code)

        Do you mind attach the current reflection patch?

        Show
        Roger Schildmeijer added a comment - Oh sweet lord. Even worse than I could imagine. Reflection is slow. QED. Regarding clone: I guess that means that we will have the clone method abstract in the RequestHandler class (to make it "easier" for third parties)? Like: protected abstract Object clone() throws CloneNotSupportedException; One different approach is to let the concrete RequestHandlers be, and use the fact that HttpRequest is unique for each request and providoe something similar to HttpSessions. Thoughts? (pros: less impact to existing code) Do you mind attach the current reflection patch?
        Hide
        Johnathan Meehan added a comment - - edited

        Do you mind attach the current reflection patch?

        • The unit tests are broken, but execution skipping these should be okay. To keep things simple I assumed (and created as needed) public access to the implementation.

        Regarding clone: I guess that means that we will have the clone method abstract in the RequestHandler class (to make it "easier" for third parties)? Like:
        protected abstract Object clone() throws CloneNotSupportedException;

        • Sure, with a call to clone inside. I wonder if most implementations would even care to override this.

        One different approach is to let the concrete RequestHandlers be, and use the fact that HttpRequest is unique for each request and providoe something similar to HttpSessions. Thoughts? (pros: less impact to existing code)

        • I'll need to think about that before I comment.
        Show
        Johnathan Meehan added a comment - - edited Do you mind attach the current reflection patch? The unit tests are broken, but execution skipping these should be okay. To keep things simple I assumed (and created as needed) public access to the implementation. Regarding clone: I guess that means that we will have the clone method abstract in the RequestHandler class (to make it "easier" for third parties)? Like: protected abstract Object clone() throws CloneNotSupportedException; Sure, with a call to clone inside. I wonder if most implementations would even care to override this. One different approach is to let the concrete RequestHandlers be, and use the fact that HttpRequest is unique for each request and providoe something similar to HttpSessions. Thoughts? (pros: less impact to existing code) I'll need to think about that before I comment.
        Hide
        Johnathan Meehan added a comment -

        I thought about the final approach mentioned Roger, but I'm not sure I'm following. Do you mean to maintain a list of running requests and control access?

        Show
        Johnathan Meehan added a comment - I thought about the final approach mentioned Roger, but I'm not sure I'm following. Do you mean to maintain a list of running requests and control access?
        Hide
        Roger Schildmeijer added a comment -

        I didn't think too much about it. But something like that, yes...I'm not sure I like it though. Got the idea from how java servlets uses HttpSessions objects.

        Show
        Roger Schildmeijer added a comment - I didn't think too much about it. But something like that, yes...I'm not sure I like it though. Got the idea from how java servlets uses HttpSessions objects.
        Hide
        Roger Schildmeijer added a comment -

        applied deft_142_reflection.patch locally on my mba (i5) machine.

        ab -k -c25 -n100000 http://127.0.0.1:8080/ gave
        before: Requests per second: 12656.56 /sec (mean)
        after: Requests per second: 5377.28 /sec (mean)

        Show
        Roger Schildmeijer added a comment - applied deft_142_reflection.patch locally on my mba (i5) machine. ab -k -c25 -n100000 http://127.0.0.1:8080/ gave before: Requests per second: 12656.56 /sec (mean) after: Requests per second: 5377.28 /sec (mean)
        Hide
        Johnathan Meehan added a comment - - edited

        Are you looking at this now, Roger? If not, I would like to put together an initial protoype-based attempt for discussion. From the stats. above, the scratch version was fast so it would be worthwhile looking at I think.

        Show
        Johnathan Meehan added a comment - - edited Are you looking at this now, Roger? If not, I would like to put together an initial protoype-based attempt for discussion. From the stats. above, the scratch version was fast so it would be worthwhile looking at I think.
        Hide
        Roger Schildmeijer added a comment -

        Not looking at this. Just "verified" your numbers (with patch applied) on my machine

        Show
        Roger Schildmeijer added a comment - Not looking at this. Just "verified" your numbers (with patch applied) on my machine
        Hide
        Roger Schildmeijer added a comment -

        "the scratch version", which version is this? the clone version?

        Show
        Roger Schildmeijer added a comment - "the scratch version", which version is this? the clone version?
        Hide
        Johnathan Meehan added a comment - - edited

        Yes; the quick implementation from which the "cloned" figures above were pulled. Didn't include a patch as it was just a little experiment.

        Show
        Johnathan Meehan added a comment - - edited Yes; the quick implementation from which the "cloned" figures above were pulled. Didn't include a patch as it was just a little experiment.
        Hide
        Johnathan Meehan added a comment -

        Naive implementation "deft-142_clone.patch"; work in progress.

        Show
        Johnathan Meehan added a comment - Naive implementation "deft-142_clone.patch"; work in progress.
        Hide
        Johnathan Meehan added a comment -

        Anybody mind taking a quick look at "deft-142_clone.patch"? Looking at it now I am wondering if it is along the right lines... perhaps I missed something.
        Assuming this is a decent start I was thinking that we could bring this change to a single method, getHandler(HttpRequest), where we provide the functionality to modifiy "internal" handlers such as "BadRequestHandler". Struck me that people might well want to provide customised handlers for some of the requests and now could be an opportune time to provide that facility.

        Show
        Johnathan Meehan added a comment - Anybody mind taking a quick look at "deft-142_clone.patch"? Looking at it now I am wondering if it is along the right lines... perhaps I missed something. Assuming this is a decent start I was thinking that we could bring this change to a single method, getHandler(HttpRequest), where we provide the functionality to modifiy "internal" handlers such as "BadRequestHandler". Struck me that people might well want to provide customised handlers for some of the requests and now could be an opportune time to provide that facility.
        Hide
        Michele Zuccalà added a comment -

        The idea it's good, and, in my opinion, could be better whit mixed handling
        In other words, if the developer map, a certain type of RequestHandler, Deft creates new clone of that, otherwise use the same.

        > Struck me that people might well want to provide customised handlers for some of the requests and now could be an
        > opportune time to provide that facility.

        In my project, this is a problem that I must face

        Show
        Michele Zuccalà added a comment - The idea it's good, and, in my opinion, could be better whit mixed handling In other words, if the developer map, a certain type of RequestHandler, Deft creates new clone of that, otherwise use the same. > Struck me that people might well want to provide customised handlers for some of the requests and now could be an > opportune time to provide that facility. In my project, this is a problem that I must face
        Hide
        Johnathan Meehan added a comment -

        > In other words, if the developer map, a certain type of RequestHandler, Deft creates new clone of that, otherwise use the same.

        Right. What I was thinking was along the lines of providing a genric "StatusRequestHandler" that would replace the current concrete, internal request handlers. I create a Map of HTTP status codes (from an enumeration) and associated request handlers and populate during startup. Then I extend the factory we have with a new method that pulls from the Map - where null, create a new instance of the generic handler with the status code passed to it; where available, return the set handler. Seems to be working okay so far, so I think I will complete that work as part of this requirement and we'll see what the thoughts are when it's finished.

        Show
        Johnathan Meehan added a comment - > In other words, if the developer map, a certain type of RequestHandler, Deft creates new clone of that, otherwise use the same. Right. What I was thinking was along the lines of providing a genric "StatusRequestHandler" that would replace the current concrete, internal request handlers. I create a Map of HTTP status codes (from an enumeration) and associated request handlers and populate during startup. Then I extend the factory we have with a new method that pulls from the Map - where null, create a new instance of the generic handler with the status code passed to it; where available, return the set handler. Seems to be working okay so far, so I think I will complete that work as part of this requirement and we'll see what the thoughts are when it's finished.
        Hide
        Johnathan Meehan added a comment -

        Ongoing work; attaching for safekeeping: Dynamic creation of request handlers; user-defined handlers for status (needs to be discussed a little).

        Show
        Johnathan Meehan added a comment - Ongoing work; attaching for safekeeping: Dynamic creation of request handlers; user-defined handlers for status (needs to be discussed a little).
        Hide
        Michele Zuccalà added a comment -

        I've problems to apply the patch, but I think that is very cool.
        My only suggestion is to modify the RequestHandlerFactory#cloneHandler method with something like that:

        if (handler != null) && !(handler instanceof ISomeInterface) {
        try

        { return (T) handler.clone(); }

        catch (CloneNotSupportedException e)

        { logger.error("Could not clone RequestHandler", e); return null; }

        }

        return handler;

        in this way, the developer can choose if the RequestHandler object must be cloned, or can be safely always the same, simply adding an interface to its implementation of a RequestHandler.

        Show
        Michele Zuccalà added a comment - I've problems to apply the patch, but I think that is very cool. My only suggestion is to modify the RequestHandlerFactory#cloneHandler method with something like that: if (handler != null) && !(handler instanceof ISomeInterface) { try { return (T) handler.clone(); } catch (CloneNotSupportedException e) { logger.error("Could not clone RequestHandler", e); return null; } } return handler; in this way, the developer can choose if the RequestHandler object must be cloned, or can be safely always the same, simply adding an interface to its implementation of a RequestHandler.
        Hide
        Johnathan Meehan added a comment -

        Notes:

        • Request handlers are created through clone (see RequestHandlerFactory and callers), except for StaticRequestHandler which I left alone for now.
        • Specific request handlers can be attached to status codes (see CustomStatusRequestHandlerExample); right now, the setting for this pairs directly to HttpStatus. This means that it is possible to set a handler for a status code that will not be invoked - confusing, but for a first run I thought it was acceptable. If people like the general idea and it is developed further, it can be refined with a consideration of the whole application flow.
        • Michele's comment noted as a future option, but not included in this patch.
        • Sundry clean-up along the way, including a little reorganisation of example packages. Ignore for now as that's DEFT-175, here because of the trouble creating the patch.

        I think I would like to make some minor changes, and add unit tests. Nonetheless, could you review and let me know what you think?

        Show
        Johnathan Meehan added a comment - Notes: Request handlers are created through clone (see RequestHandlerFactory and callers), except for StaticRequestHandler which I left alone for now. Specific request handlers can be attached to status codes (see CustomStatusRequestHandlerExample); right now, the setting for this pairs directly to HttpStatus. This means that it is possible to set a handler for a status code that will not be invoked - confusing, but for a first run I thought it was acceptable. If people like the general idea and it is developed further, it can be refined with a consideration of the whole application flow. Michele's comment noted as a future option, but not included in this patch. Sundry clean-up along the way, including a little reorganisation of example packages. Ignore for now as that's DEFT-175 , here because of the trouble creating the patch. I think I would like to make some minor changes, and add unit tests. Nonetheless, could you review and let me know what you think?
        Hide
        Roger Schildmeijer added a comment -

        Review in my queue

        Show
        Roger Schildmeijer added a comment - Review in my queue
        Hide
        Roger Schildmeijer added a comment -

        could not apply patch against trunk (sandbox).

        Show
        Roger Schildmeijer added a comment - could not apply patch against trunk (sandbox).
        Hide
        Roger Schildmeijer added a comment -

        (from the top of my head).

        I don't know how you do a proper "rebase" with SVN if a patch doesn't apply...if you are having problem with this johnathan, let me know and we will sort this out together

        Show
        Roger Schildmeijer added a comment - (from the top of my head). I don't know how you do a proper "rebase" with SVN if a patch doesn't apply...if you are having problem with this johnathan, let me know and we will sort this out together
        Hide
        Emmanuel Lecharny added a comment -

        Roger,

        what's the problem you have with SVN ?

        Show
        Emmanuel Lecharny added a comment - Roger, what's the problem you have with SVN ?
        Hide
        Emmanuel Lecharny added a comment -

        hmmm, I tried to apply the patch, and, yes, it blows chunks...

        That's the problem with all those pending patches, created against an old verion of the source, when we have already applied some of them. SVN sucks in this very case.

        I'm afraid you have to go through a manual merge :/

        Show
        Emmanuel Lecharny added a comment - hmmm, I tried to apply the patch, and, yes, it blows chunks... That's the problem with all those pending patches, created against an old verion of the source, when we have already applied some of them. SVN sucks in this very case. I'm afraid you have to go through a manual merge :/
        Hide
        Johnathan Meehan added a comment - - edited

        I have just tried and am having the same trouble. I'll create another patch this evening after work with everything merged, and we'll see. Iif anybody wants to spend their time instead looking at DEFT-100...

        Show
        Johnathan Meehan added a comment - - edited I have just tried and am having the same trouble. I'll create another patch this evening after work with everything merged, and we'll see. Iif anybody wants to spend their time instead looking at DEFT-100 ...
        Hide
        Emmanuel Lecharny added a comment -

        Frankly, it's probably a better idea to just work in sandbox now.

        Anyone can review the commit in the M. Creating patches and adding them to JIRA is a waste of time at this point... (IMHO)

        Show
        Emmanuel Lecharny added a comment - Frankly, it's probably a better idea to just work in sandbox now. Anyone can review the commit in the M. Creating patches and adding them to JIRA is a waste of time at this point... (IMHO)
        Hide
        Roger Schildmeijer added a comment -

        I like the idea. Lets go for CTR (commit the review) until further notice

        Show
        Roger Schildmeijer added a comment - I like the idea. Lets go for CTR (commit the review) until further notice
        Hide
        Johnathan Meehan added a comment -

        Initial implementation committed: http://svn.apache.org/viewvc?rev=1154336&view=rev

        Notes:

        • Did not include the status handler implementation that is in the patch; wanted to discuss that first, and keep this change as one thing (DEFT-181). Other than that, comments from "30/Jul/11 13:30" generally apply.
        • Introduced Mockito (http://mockito.org/) to the build, which is under the MIT licence.
        • Reorganised a few things along the way.
        • Added unit tests, but not for some types or areas (e.g. ForbiddenRequestHandler) that will be removed when implementing DEFT-181; will revisit if that does not go ahead.
        • Would like to revisit Application#getHandler(String) as part of DEFT-181 or if not implementing.

        Test:
        ab -k -c10 -n250000 http://127.0.0.1:8080/

        14021.15 /sec (mean)
        14132.04 /sec (mean)
        16253.50 /sec (mean)
        14880.28 /sec (mean)
        15084.88 /sec (mean)

        Ready for review.

        Show
        Johnathan Meehan added a comment - Initial implementation committed: http://svn.apache.org/viewvc?rev=1154336&view=rev Notes: Did not include the status handler implementation that is in the patch; wanted to discuss that first, and keep this change as one thing ( DEFT-181 ). Other than that, comments from "30/Jul/11 13:30" generally apply. Introduced Mockito ( http://mockito.org/ ) to the build, which is under the MIT licence. Reorganised a few things along the way. Added unit tests, but not for some types or areas (e.g. ForbiddenRequestHandler) that will be removed when implementing DEFT-181 ; will revisit if that does not go ahead. Would like to revisit Application#getHandler(String) as part of DEFT-181 or if not implementing. Test: ab -k -c10 -n250000 http://127.0.0.1:8080/ 14021.15 /sec (mean) 14132.04 /sec (mean) 16253.50 /sec (mean) 14880.28 /sec (mean) 15084.88 /sec (mean) Ready for review.

          People

          • Assignee:
            Johnathan Meehan
            Reporter:
            Niklas Gustavsson
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development