Wicket
  1. Wicket
  2. WICKET-2705

Feedback messages get cleaned up in AJAX request, thus never rendered and never visible to user

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.4.6
    • Fix Version/s: 6.0.0-beta1
    • Component/s: wicket
    • Labels:
      None
    • Environment:
      Sun Glassfish Enterpriser Server 2.1

      Description

      Consider a page containing a form and an AJAXy component (as in an AJAX link or a panel with AjaxSelfUpdatingTimerBehavior). The form contains code, that makes its processing take relatively long time, even when validation fails (let's say we forgot to fill in a requred field). The form gets submitted, and while it's processing, the AJAX component triggers a request (AJAX link gets clicked, or AjaxSelfUpdatingTimerBehavior kicks in). While the AJAX request gets served, cleanupFeedbackMessages() is called, wiping all feedback messages found by WebSession.MESSAGES_FOR_COMPONENTS filter.

      WebSession.MESSAGES_FOR_COMPONENTS implementation (or its usage in cleanupFeedbackMessages()) is a little bit fishy, as it accept()s messages, that haven't been rendered.

      Will add testcase.

      1. feedbackbug.patch
        0.5 kB
        Jan Winkler
      2. feedbackbug.tgz
        3 kB
        Jan Winkler
      3. patch.txt
        3 kB
        Martijn Dashorst

        Issue Links

          Activity

          Jan Winkler created issue -
          Hide
          Jan Winkler added a comment - - edited

          Testcase - maven project quickly hacked together.

          1) Unpack, build, and deploy the project to appserver (I'm running on SGES 2.1)
          2) Open the application (e.g. http://localhost:8080/feedbackbug-1.0-SNAPSHOT/ )
          3) DON'T fill in the text field, submit the form, its onError will hang for 10 secs.
          4) In the 10 second meantime, click the 'clickme' button
          5) Wait for the end of the 10 sec processing. No feedback messages are provided, even though one should be displayed.

          Show
          Jan Winkler added a comment - - edited Testcase - maven project quickly hacked together. 1) Unpack, build, and deploy the project to appserver (I'm running on SGES 2.1) 2) Open the application (e.g. http://localhost:8080/feedbackbug-1.0-SNAPSHOT/ ) 3) DON'T fill in the text field, submit the form, its onError will hang for 10 secs. 4) In the 10 second meantime, click the 'clickme' button 5) Wait for the end of the 10 sec processing. No feedback messages are provided, even though one should be displayed.
          Jan Winkler made changes -
          Field Original Value New Value
          Attachment feedbackbug.tgz [ 12431665 ]
          Hide
          Jan Winkler added a comment -

          Attached patch solves my problem.

          WebSession.MESSAGES_FOR_COMPONENTS does not acccept() messages that haven't been rendered yet.

          Show
          Jan Winkler added a comment - Attached patch solves my problem. WebSession.MESSAGES_FOR_COMPONENTS does not acccept() messages that haven't been rendered yet.
          Jan Winkler made changes -
          Attachment feedbackbug.patch [ 12431668 ]
          Hide
          Juergen Donnerstag added a comment -

          By now 1.3 issues only get fixed if is security releated or a big big bug. Please consider using 1.4 instead

          Show
          Juergen Donnerstag added a comment - By now 1.3 issues only get fixed if is security releated or a big big bug. Please consider using 1.4 instead
          Jan Winkler made changes -
          Affects Version/s 1.4.6 [ 12314470 ]
          Affects Version/s 1.3.7 [ 12313924 ]
          Jan Winkler made changes -
          Comment [ Quick peek at wicket trunk shows that the same problem affects 1.4.

          See http://svn.apache.org/repos/asf/wicket/branches/wicket-1.4.x/wicket/src/main/java/org/apache/wicket/protocol/http/WebSession.java ]
          Jan Winkler made changes -
          Affects Version/s 1.3.7 [ 12313924 ]
          Affects Version/s 1.4.6 [ 12314470 ]
          Hide
          Jan Winkler added a comment -

          Affects 1.4.x as well.

          Show
          Jan Winkler added a comment - Affects 1.4.x as well.
          Jan Winkler made changes -
          Affects Version/s 1.4.6 [ 12314470 ]
          Affects Version/s 1.3.7 [ 12313924 ]
          Hide
          Jan Winkler added a comment - - edited

          The patch attached earlier applies to current trunk.

          wicket/src/main/java (trunk)$ patch -p1 < feedbackbug.patch
          patching file org/apache/wicket/protocol/http/WebSession.java
          Hunk #1 succeeded at 50 with fuzz 1 (offset 5 lines).

          Show
          Jan Winkler added a comment - - edited The patch attached earlier applies to current trunk. wicket/src/main/java (trunk)$ patch -p1 < feedbackbug.patch patching file org/apache/wicket/protocol/http/WebSession.java Hunk #1 succeeded at 50 with fuzz 1 (offset 5 lines).
          Hide
          Igor Vaynberg added a comment -

          the proposed patch will not work. it basically makes all calls to component.info() equivalent to getsession().info() which are two completely different scopes. the patch breaks semantics and will affect processing of feedback messages during non-ajax requests.

          i dont see an easy solution to this problem. in order to support it fully we have to store the messages with the page or possibly pagemap - which is the synchronization token for concurrent requests.

          Show
          Igor Vaynberg added a comment - the proposed patch will not work. it basically makes all calls to component.info() equivalent to getsession().info() which are two completely different scopes. the patch breaks semantics and will affect processing of feedback messages during non-ajax requests. i dont see an easy solution to this problem. in order to support it fully we have to store the messages with the page or possibly pagemap - which is the synchronization token for concurrent requests.
          Hide
          Martijn Dashorst added a comment -

          AFAIK the patch is valid. It doesn't modify the scope, it only modifies that just the rendered feedback messages are cleared, and keeps the non-rendered messages for another time.

          This concurrency issue is something we encountered as well: a normal form submission and an ajax event that are fired at the same time, where the ajax request is canceled with an EmptyAjaxRequestTarget (because normal page processing is already in progress). When the RC.detach() is processed, it clears all feedback messages.

          Of course this shouldn't happen outside the synchronized block (currently the feedback messages are cleared outside the sync'd block), but with this patch threading issues should be less of an issue than before.

          Show
          Martijn Dashorst added a comment - AFAIK the patch is valid. It doesn't modify the scope, it only modifies that just the rendered feedback messages are cleared, and keeps the non-rendered messages for another time. This concurrency issue is something we encountered as well: a normal form submission and an ajax event that are fired at the same time, where the ajax request is canceled with an EmptyAjaxRequestTarget (because normal page processing is already in progress). When the RC.detach() is processed, it clears all feedback messages. Of course this shouldn't happen outside the synchronized block (currently the feedback messages are cleared outside the sync'd block), but with this patch threading issues should be less of an issue than before.
          Hide
          Martijn Dashorst added a comment -

          Problem is that this patch also kills manual cleanup of feedbackmessages.

          When applied to branch/14x the patch causes a couple of testcase failures, most of which can be fixed by calling webSession.cleanupFeedbackmessages() in MockWebApplication#setupRequestAndResponse() (is intended behavior: all rendered feedbackmessages are cleared, just like in normal request processing).

          The only test that fails is: WicketTesterTest#testToggleAjaxFormButton() and that is because the MockAjaxFormPage doesn't provide a feedbackpanel. Adding the feedbackpanel.

          Session#cleanupFeedbackMessages() is documented as:

          /**

          • Cleans up all rendered feedback messages and any unrendered, dangling feedback messages there may be left after that.
            */

          But I think we should only clean up rendered feedback messages.

          Show
          Martijn Dashorst added a comment - Problem is that this patch also kills manual cleanup of feedbackmessages. When applied to branch/14x the patch causes a couple of testcase failures, most of which can be fixed by calling webSession.cleanupFeedbackmessages() in MockWebApplication#setupRequestAndResponse() (is intended behavior: all rendered feedbackmessages are cleared, just like in normal request processing). The only test that fails is: WicketTesterTest#testToggleAjaxFormButton() and that is because the MockAjaxFormPage doesn't provide a feedbackpanel. Adding the feedbackpanel. Session#cleanupFeedbackMessages() is documented as: /** Cleans up all rendered feedback messages and any unrendered, dangling feedback messages there may be left after that. */ But I think we should only clean up rendered feedback messages.
          Hide
          Martijn Dashorst added a comment -

          Patch that fixes all tests and fixes the cleanup. Remaining issue: what to do about the modified semantics of cleanupFeedbackmessages()

          Show
          Martijn Dashorst added a comment - Patch that fixes all tests and fixes the cleanup. Remaining issue: what to do about the modified semantics of cleanupFeedbackmessages()
          Martijn Dashorst made changes -
          Attachment patch.txt [ 12435311 ]
          Hide
          Martijn Dashorst added a comment -

          Another hurdle to take: FeedbackMessage#reporter is Component, and should be nulled between requests.

          Show
          Martijn Dashorst added a comment - Another hurdle to take: FeedbackMessage#reporter is Component, and should be nulled between requests.
          Hide
          Jan Winkler added a comment -

          My patch indeed breaks things – touching semantics of cleanupFeedbackmessages() is not a good idea as feedback messages also serve to signal FormComponent's validity. WIth my patch, FormComponents that failed to verify during an ajax request, are treated not valid until the error message is actually displayed to the user. This results in displaying error messages on submitting correctly filled in Form.

          The problem I originally reported is exactly what Martijn said: "a normal form submission and an ajax event that are fired at the same time, where the ajax request is canceled with an EmptyAjaxRequestTarget (because normal page processing is already in progress). When the RC.detach() is processed, it clears all feedback messages."

          Cancelling an ajax request because normal page processing is in progress seems like an intentional no-op, and it should be. Cleaning feedback messages is not a no-op though.

          Show
          Jan Winkler added a comment - My patch indeed breaks things – touching semantics of cleanupFeedbackmessages() is not a good idea as feedback messages also serve to signal FormComponent's validity. WIth my patch, FormComponents that failed to verify during an ajax request, are treated not valid until the error message is actually displayed to the user. This results in displaying error messages on submitting correctly filled in Form. The problem I originally reported is exactly what Martijn said: "a normal form submission and an ajax event that are fired at the same time, where the ajax request is canceled with an EmptyAjaxRequestTarget (because normal page processing is already in progress). When the RC.detach() is processed, it clears all feedback messages." Cancelling an ajax request because normal page processing is in progress seems like an intentional no-op, and it should be. Cleaning feedback messages is not a no-op though.
          Florian Wunderlich made changes -
          Link This issue relates to WICKET-2948 [ WICKET-2948 ]
          Igor Vaynberg made changes -
          Link This issue is depended upon by WICKET-2948 [ WICKET-2948 ]
          Igor Vaynberg made changes -
          Assignee Igor Vaynberg [ ivaynberg ]
          Igor Vaynberg made changes -
          Fix Version/s 1.5-M3 [ 12315329 ]
          Igor Vaynberg made changes -
          Fix Version/s 1.6.0 [ 12315431 ]
          Fix Version/s 1.5-M3 [ 12315329 ]
          Igor Vaynberg made changes -
          Link This issue incorporates WICKET-499 [ WICKET-499 ]
          Hide
          Igor Vaynberg added a comment -

          a proper solution to this will look something like this:

          feedback messages are stored in component's metadata with the components
          add new support for persistent feedback messages - these are not cleaned up after render

          form components report their errors as persistent
          form components clean their reported errors once validated
          so form component workflow looks like this:

          before validate - clear all reported errors
          validate - reports persistent messages
          onvalid - clear all reported errors
          onerror - do nothing, messages stay until next validation or onvalid

          Show
          Igor Vaynberg added a comment - a proper solution to this will look something like this: feedback messages are stored in component's metadata with the components add new support for persistent feedback messages - these are not cleaned up after render form components report their errors as persistent form components clean their reported errors once validated so form component workflow looks like this: before validate - clear all reported errors validate - reports persistent messages onvalid - clear all reported errors onerror - do nothing, messages stay until next validation or onvalid
          Hide
          Igor Vaynberg added a comment -

          the above solution is a little too much for 1.5 because we want to get it out asap, so scheduling for 1.6.0

          Show
          Igor Vaynberg added a comment - the above solution is a little too much for 1.5 because we want to get it out asap, so scheduling for 1.6.0
          Igor Vaynberg made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]

            People

            • Assignee:
              Igor Vaynberg
              Reporter:
              Jan Winkler
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development