Wicket
  1. Wicket
  2. WICKET-5387

Page#onInitialize called after an exception in the constructor of Page

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 6.11.0
    • Fix Version/s: 6.13.0, 7.0.0-M1
    • Component/s: wicket
    • Labels:
      None
    • Environment:
      Linux

      Description

      Page#onInitialize is called when the constructor of Page throws an exception, i.e. when the Page is not initialized correctly. This can cause additional exceptions which are usually added to an error log even in cases in which the exception in the constructor is handled (by Wicket). This issue is possibly related to WICKET-5083.

      Another case of the problem occurs when setResponsePage(...) is used in the constructor to navigate away from a page that can't be initialized correctly.

      I'm attaching a quickstart.

      1. WICKET-5387.patch
        0.8 kB
        Martin Grigorov
      2. 5387.1.tar.gz
        20 kB
        Walter B. Rasmann
      3. 5387.tar.gz
        18 kB
        Walter B. Rasmann

        Issue Links

          Activity

          Hide
          Sven Meier added a comment -

          Thanks Martin!

          Show
          Sven Meier added a comment - Thanks Martin!
          Hide
          Martin Grigorov added a comment -

          I'm closing the ticket for now.
          Please reopen if you find a use case where the applied fix breaks the functionality.

          Show
          Martin Grigorov added a comment - I'm closing the ticket for now. Please reopen if you find a use case where the applied fix breaks the functionality.
          Hide
          Martin Grigorov added a comment -

          > What about pages which are instantiated but not rendered immediately, e.g. rendered after a redirect only?

          I am not able to find a problem with the suggested patch with REDIRECT_TO_RENDER strategy.
          Due to the check at https://github.com/apache/wicket/blob/wicket-6.x/wicket-core/src/main/java/org/apache/wicket/request/handler/render/WebPageRenderer.java#L209 the page is rendered in the first request because the current and the target urls are the same. They differ only when there is an action (onClick, onEvent, etc.). That is, only when there is an action Wicket will redirect to render the page.

          Show
          Martin Grigorov added a comment - > What about pages which are instantiated but not rendered immediately, e.g. rendered after a redirect only? I am not able to find a problem with the suggested patch with REDIRECT_TO_RENDER strategy. Due to the check at https://github.com/apache/wicket/blob/wicket-6.x/wicket-core/src/main/java/org/apache/wicket/request/handler/render/WebPageRenderer.java#L209 the page is rendered in the first request because the current and the target urls are the same. They differ only when there is an action (onClick, onEvent, etc.). That is, only when there is an action Wicket will redirect to render the page.
          Hide
          Martin Grigorov added a comment -

          The proposed patch is applied and will be part of 6.13.
          I'll debug how REDIRECT_TO_RENDER strategy behaves with it.

          Show
          Martin Grigorov added a comment - The proposed patch is applied and will be part of 6.13. I'll debug how REDIRECT_TO_RENDER strategy behaves with it.
          Hide
          Walter B. Rasmann added a comment -

          Thank you very much for having a look at it and for the patch. I have tested it on top of wicket-6.11.0. Everything seems to work so far. If the exception occures in onInitialize() the page isn't saved either (though super.onInitialize() is called first), so the problem with the exceptions in onBeforeRender() does not happen.

          Show
          Walter B. Rasmann added a comment - Thank you very much for having a look at it and for the patch. I have tested it on top of wicket-6.11.0. Everything seems to work so far. If the exception occures in onInitialize() the page isn't saved either (though super.onInitialize() is called first), so the problem with the exceptions in onBeforeRender() does not happen.
          Hide
          Sven Meier added a comment -

          With "page1?0" you're just pulling the previous page instance out of the store, which failed previously on instantiation.

          With Martin's patch the failed page is no longer stored, so this is no longer possible.

          Show
          Sven Meier added a comment - With "page1?0" you're just pulling the previous page instance out of the store, which failed previously on instantiation. With Martin's patch the failed page is no longer stored, so this is no longer possible.
          Hide
          Walter B. Rasmann added a comment - - edited

          Today I noticed that a call to onBeforeRender() of an uninitialized page instance can be triggered by calling the page with the "?0" query string more than once.

          It is possible to reproduce this with the quickstart by calling "http://localhost:8080/page1?0" multiple times during one session or by first calling "http://localhost:8080/page1" and calling "http://localhost:8080/page1?0" after that.

          Show
          Walter B. Rasmann added a comment - - edited Today I noticed that a call to onBeforeRender() of an uninitialized page instance can be triggered by calling the page with the "?0" query string more than once. It is possible to reproduce this with the quickstart by calling "http://localhost:8080/page1?0" multiple times during one session or by first calling "http://localhost:8080/page1" and calling "http://localhost:8080/page1?0" after that.
          Hide
          Sven Meier added a comment -

          Hm, maybe with your patch the following "dirty hack" in Page#init() is obsolete?

          // this is a bit of a dirty hack, but calling dirty(true) results in isStateless called
          // which is bound to set the stateless cache to true as there are no components yet
          stateless = null;

          BTW can anybody make sense of the comment a few lines earlier? Does it still hold then?

          // All Pages are born dirty so they get clustered right away
          dirty(true);

          And why is #dirty(boolean) public anyway?

          Show
          Sven Meier added a comment - Hm, maybe with your patch the following "dirty hack" in Page#init() is obsolete? // this is a bit of a dirty hack, but calling dirty(true) results in isStateless called // which is bound to set the stateless cache to true as there are no components yet stateless = null; BTW can anybody make sense of the comment a few lines earlier? Does it still hold then? // All Pages are born dirty so they get clustered right away dirty(true); And why is #dirty(boolean) public anyway?
          Hide
          Martin Grigorov added a comment -

          Right.
          It is easy to mark it as dirty before the redirect but it seems REDIRECT_TO_RENDER strategy is broken anyway...
          Just create a simple stateful page, configure the redirect strategy and you'll see no redirect happens.

          Show
          Martin Grigorov added a comment - Right. It is easy to mark it as dirty before the redirect but it seems REDIRECT_TO_RENDER strategy is broken anyway... Just create a simple stateful page, configure the redirect strategy and you'll see no redirect happens.
          Hide
          Sven Meier added a comment -

          What about pages which are instantiated but not rendered immediately, e.g. rendered after a redirect only?

          Show
          Sven Meier added a comment - What about pages which are instantiated but not rendered immediately, e.g. rendered after a redirect only?
          Hide
          Martin Grigorov added a comment - - edited

          Here is a patch that solves the problem.
          The idea is that initially the page is "touched" in o.a.w.Page#onInitialize().
          This way pages which haven't been rendered will not be stored at the end of the request.

          Show
          Martin Grigorov added a comment - - edited Here is a patch that solves the problem. The idea is that initially the page is "touched" in o.a.w.Page#onInitialize(). This way pages which haven't been rendered will not be stored at the end of the request.
          Hide
          Martin Grigorov added a comment -

          Thanks for fixing the quickstart!

          I see what happens:

          • the page is added to org.apache.wicket.page.RequestAdapter#touchedPages list in o.a.w.Page's constructor.
          • the exception is thrown in Page1's constructor
          • Wicket renders InternalErrorPage
          • at the end of the request all touched pages are saved in the disk if they are stateful. To check whether a page is stateful Wicket initializes it if it is not initialized already

          It is a bug for sure. I have no solution for now.

          Show
          Martin Grigorov added a comment - Thanks for fixing the quickstart! I see what happens: the page is added to org.apache.wicket.page.RequestAdapter#touchedPages list in o.a.w.Page's constructor. the exception is thrown in Page1's constructor Wicket renders InternalErrorPage at the end of the request all touched pages are saved in the disk if they are stateful. To check whether a page is stateful Wicket initializes it if it is not initialized already It is a bug for sure. I have no solution for now.
          Hide
          Walter B. Rasmann added a comment -

          Thank you for your response.

          I have added a Start.java and a homepage.

          Please use the following URLs to test:

          http://localhost:8080/page1
          http://localhost:8080/page2
          http://localhost:8080/page3
          http://localhost:8080/page4

          Show
          Walter B. Rasmann added a comment - Thank you for your response. I have added a Start.java and a homepage. Please use the following URLs to test: http://localhost:8080/page1 http://localhost:8080/page2 http://localhost:8080/page3 http://localhost:8080/page4
          Hide
          Martin Grigorov added a comment -

          The quickstart is broken.
          1) It has no home page
          2) it has no Start.java

          Please make sure the quickstart works so we can help you.

          Show
          Martin Grigorov added a comment - The quickstart is broken. 1) It has no home page 2) it has no Start.java Please make sure the quickstart works so we can help you.
          Hide
          Martin Grigorov added a comment -

          setResponsePage() just schedules another page to be executed, but it doesn't stop the execution flow.
          To stop the flow you can use "throw new RestartResponseException(...)". This uses an exception and thus the execution stops where it is thrown.

          I'll check your quickstart now.

          Show
          Martin Grigorov added a comment - setResponsePage() just schedules another page to be executed, but it doesn't stop the execution flow. To stop the flow you can use "throw new RestartResponseException(...)". This uses an exception and thus the execution stops where it is thrown. I'll check your quickstart now.

            People

            • Assignee:
              Martin Grigorov
              Reporter:
              Walter B. Rasmann
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development