Details

    • Type: Sub-task Sub-task
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.5-RC7
    • Fix Version/s: 1.5.1
    • Component/s: wicket
    • Labels:
      None

      Description

      From the investigation in WICKET-4009 it seems that RequestLogger leaks PageAccessSynchronizer.PageLock's after itself.
      We need to investigate and improve.

      1. WICKET-4029.patch
        2 kB
        Andrea Del Bene

        Activity

        Hide
        Andrea Del Bene added a comment -

        Some further informations.
        RequestLogger.requestTime is called at the end of RequestCycle.onInternalDetach. requestTime calls some methods of RequestLogger, including method getListenerString. One of the first instructions of this method is ListenerInterfaceRequestHandler.getPageClass() and this is causes an indirect call to PageAccessSynchronizer.getPage(id). If you need I can post the whole call stack.

        Possible solution (not tested yet!):

        Before throwing PageExpiredException call IPageManager.commitRequest (easy to implement, riskless, and without overhead)

        Obviously I need your opinion

        Show
        Andrea Del Bene added a comment - Some further informations. RequestLogger.requestTime is called at the end of RequestCycle.onInternalDetach. requestTime calls some methods of RequestLogger, including method getListenerString. One of the first instructions of this method is ListenerInterfaceRequestHandler.getPageClass() and this is causes an indirect call to PageAccessSynchronizer.getPage(id). If you need I can post the whole call stack. Possible solution (not tested yet!): Before throwing PageExpiredException call IPageManager.commitRequest (easy to implement, riskless, and without overhead) Obviously I need your opinion
        Hide
        Andrea Del Bene added a comment -

        Sorry,

        I didn' red the issue title properly. The solution proposed doesn't prevent all RequestLogger leaks.

        Show
        Andrea Del Bene added a comment - Sorry, I didn' red the issue title properly. The solution proposed doesn't prevent all RequestLogger leaks.
        Hide
        Andrea Del Bene added a comment -

        RequestLogger.requestTime JavaDoc says:

        "This method is called when the request is over. This will set the total time a request takes and cleans up the current request data."

        So we could avoid to invoke it inside RequestCycle.onInternalDetach and delegate its calling to the AbstractRequestCycleListener added by Application.createRequestCycle (see proposed patch).

        RequestCycle.onDetach code ensures that listeners onEndRequest is called before IPageManager.commitRequest().

        Show
        Andrea Del Bene added a comment - RequestLogger.requestTime JavaDoc says: "This method is called when the request is over. This will set the total time a request takes and cleans up the current request data." So we could avoid to invoke it inside RequestCycle.onInternalDetach and delegate its calling to the AbstractRequestCycleListener added by Application.createRequestCycle (see proposed patch). RequestCycle.onDetach code ensures that listeners onEndRequest is called before IPageManager.commitRequest().
        Hide
        Martin Grigorov added a comment -

        The patch looks OK to me but I know from Emond that Topicus also have some issues with RequestLogger.
        Assigning the ticket to Martijn to see if this patch is enough to solve their issues too.

        Show
        Martin Grigorov added a comment - The patch looks OK to me but I know from Emond that Topicus also have some issues with RequestLogger. Assigning the ticket to Martijn to see if this patch is enough to solve their issues too.
        Hide
        Martin Grigorov added a comment -

        The patch is committed in trunk.

        Thanks Andrea!
        Emond also confirmed that it solves their problem.

        Show
        Martin Grigorov added a comment - The patch is committed in trunk. Thanks Andrea! Emond also confirmed that it solves their problem.
        Hide
        Andrea Del Bene added a comment -

        Thanks to you all!

        Show
        Andrea Del Bene added a comment - Thanks to you all!

          People

          • Assignee:
            Martin Grigorov
            Reporter:
            Martin Grigorov
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development