Wicket
  1. Wicket
  2. WICKET-5039

Manual invocation of FunctionsExecutor#notify() is broken

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: 6.4.0
    • Fix Version/s: 6.7.0
    • Component/s: wicket
    • Labels:
      None
    • Environment:
      any

      Description

      We are having problems on Wicket 1.4.x with and "AJAX function" processEvaluation... and I see the code is practically the same at 6.x. So, let's bring the subject to the list...

      Our problem is that after evaluating some expressions with errors, screen "freezes" because post-call handlers are not called. Problem seems to be related to the code.

      // test if the javascript is in form of identifier|code
      // if it is, we allow for letting the javascript decide when the rest of processing will continue
      // by invoking identifier();
      var res = text.match(new RegExp("^([a-z|A-Z_][a-z|A-Z|0-9_])\\|((.|
      n)
      )$"));

      if (res !== null) {
      var f = jQuery.noop;
      text = "f = function(" + res[1] + ")

      {" + res[2] + "}

      ;";

      try

      { // do the evaluation eval(text); f(notify); } catch (exception) { Wicket.Log.error("Wicket.Ajax.Call.processEvaluation: Exception evaluating javascript: " + exception + ", text: " + text); }

      }


      In case of error. Shouldn't it be

      try { // do the evaluation eval(text); f(notify); }

      catch (exception)

      { Wicket.Log.error("Wicket.Ajax.Call.processEvaluation: Exception evaluating javascript: " + exception + ", text: " + text); notify(); }

      So that next steps in processing get called. The above solves or freezing problem in case of error

      1. wicket5039.tar.gz
        21 kB
        Ernesto Reinaldo Barreiro
      2. WICKET-5039.patch
        0.6 kB
        Ernesto Reinaldo Barreiro

        Issue Links

          Activity

          Ernesto Reinaldo Barreiro created issue -
          Hide
          Ernesto Reinaldo Barreiro added a comment -

          patch fixing the issue for 6.x branch

          Show
          Ernesto Reinaldo Barreiro added a comment - patch fixing the issue for 6.x branch
          Ernesto Reinaldo Barreiro made changes -
          Field Original Value New Value
          Attachment WICKET-5039.patch [ 12569186 ]
          Ernesto Reinaldo Barreiro made changes -
          Summary ajax post call handlers might not get called because of failure on problems with processEvaluation function ajax post call handlers might not get called because of failure on AJAX processEvaluation function
          Hide
          Sven Meier added a comment -

          Out of curiosity, what are you using that "identifier|code" evaluation for?

          Show
          Sven Meier added a comment - Out of curiosity, what are you using that "identifier|code" evaluation for?
          Hide
          Ernesto Reinaldo Barreiro added a comment -

          @Sven,

          The problem appears on the site of a customer of mine: users complained of screens getting frozen. They have pinpointed that screen got frozen when exception happened on code above. I have no idea of what expressions of that kind they are using: I will ask them if they can provide an example of failing expressions. Do you need me to provide a quick-start? It is very easy to produce one.

          Show
          Ernesto Reinaldo Barreiro added a comment - @Sven, The problem appears on the site of a customer of mine: users complained of screens getting frozen. They have pinpointed that screen got frozen when exception happened on code above. I have no idea of what expressions of that kind they are using: I will ask them if they can provide an example of failing expressions. Do you need me to provide a quick-start? It is very easy to produce one.
          Hide
          Sven Meier added a comment -

          A quickstart would be nice, yes.

          I'm just wondering how to handle the case when the evaluation already called notiy(), then fails with an exception and processEvaluation() would call notify once again?

          Show
          Sven Meier added a comment - A quickstart would be nice, yes. I'm just wondering how to handle the case when the evaluation already called notiy(), then fails with an exception and processEvaluation() would call notify once again?
          Hide
          Ernesto Reinaldo Barreiro added a comment -

          I will try to produce a quickstart tomorrow morning... I have also asked my client if he can provide an example for this "use case". Or maybe some of the other core developer can shed some light about what this "special" case was introduced... I find it dangerous because of that you mention above and also because nothing prevent users from calling notify(); several times e.g. expression A|

          {A();A();A();}

          ...

          Show
          Ernesto Reinaldo Barreiro added a comment - I will try to produce a quickstart tomorrow morning... I have also asked my client if he can provide an example for this "use case". Or maybe some of the other core developer can shed some light about what this "special" case was introduced... I find it dangerous because of that you mention above and also because nothing prevent users from calling notify(); several times e.g. expression A| {A();A();A();} ...
          Hide
          Ernesto Reinaldo Barreiro added a comment -

          Sven,

          I think code

          var res = text.match(new RegExp("^([a-z|A-Z_][a-z|A-Z|0-9_])\\|((.|
          n)
          )$"));

          if (res !== null) {
          var f = jQuery.noop;
          text = "f = function(" + res[1] + ")

          {" + res[2] + "}

          ;";

          try

          { // do the evaluation eval(text); f(notify); }

          catch (exception)

          { Wicket.Log.error("Wicket.Ajax.Call.processEvaluation: Exception evaluating javascript: " + exception + ", text: " + text); }

          }

          will never get called on Wicket 6.x. I have tried the following

          <p>
          <a wicket:id="correctexpression">Correct Expression</a>
          </p>

          and

          AjaxLink<Void> correctexpression = new AjaxLink<Void>("correctexpression") {

          private static final long serialVersionUID = 1L;

          @Override
          public void onClick(AjaxRequestTarget target) {
          target.appendJavaScript("A|alert('Correct');A();");
          target.prependJavaScript("A|alert('Correct');A();");
          target.addListener(new AjaxRequestTarget.IListener() {

          @Override
          public void onBeforeRespond(Map<String, Component> map,
          AjaxRequestTarget target) {

          }

          @Override
          public void onAfterRespond(Map<String, Component> map,
          IJavaScriptResponse response)

          { response.addJavaScript("A|alert('Correct');A();"); }

          });
          }
          };
          add(correctexpression);

          but then generated JavaScript will be

          INFO:
          <?xml version="1.0" encoding="UTF-8"?><ajax-response><evaluate><![CDATA[(function()

          {A|alert('Correct');A();})();]]></evaluate><priority-evaluate><![CDATA[(function(){A|alert('Correct');A();}

          )();]]></priority-evaluate><evaluate><![CDATA[(function()

          {A|alert('Correct');A();})();]]></evaluate></ajax-response>
          ERROR: Wicket.Ajax.Call.processEvaluation: Exception evaluating javascript: ReferenceError: A is not defined, text: (function(){A|alert('Correct');A();}

          )();
          ERROR: Wicket.Ajax.Call.processEvaluation: Exception evaluating javascript: ReferenceError: A is not defined, text: (function()

          {A|alert('Correct');A();})();
          ERROR: Wicket.Ajax.Call.processEvaluation: Exception evaluating javascript: ReferenceError: A is not defined, text: (function(){A|alert('Correct');A();}

          )();
          INFO: Response processed successfully.
          INFO: refocus last focused component not needed/allowed
          INFO: focus removed from correctexpression2
          INFO: focus set on wicketDebugLink
          INFO: focus removed from wicketDebugLink

          Then because of the "anonymous" function (function()

          { ...}

          )(); around expression it will never match regular expression. So, feature A|...A(); is not really supported on 6.x

          Show
          Ernesto Reinaldo Barreiro added a comment - Sven, I think code var res = text.match(new RegExp("^( [a-z|A-Z_] [a-z|A-Z|0-9_] )\\|((.| n) )$")); if (res !== null) { var f = jQuery.noop; text = "f = function(" + res [1] + ") {" + res[2] + "} ;"; try { // do the evaluation eval(text); f(notify); } catch (exception) { Wicket.Log.error("Wicket.Ajax.Call.processEvaluation: Exception evaluating javascript: " + exception + ", text: " + text); } } will never get called on Wicket 6.x. I have tried the following <p> <a wicket:id="correctexpression">Correct Expression</a> </p> and AjaxLink<Void> correctexpression = new AjaxLink<Void>("correctexpression") { private static final long serialVersionUID = 1L; @Override public void onClick(AjaxRequestTarget target) { target.appendJavaScript("A|alert('Correct');A();"); target.prependJavaScript("A|alert('Correct');A();"); target.addListener(new AjaxRequestTarget.IListener() { @Override public void onBeforeRespond(Map<String, Component> map, AjaxRequestTarget target) { } @Override public void onAfterRespond(Map<String, Component> map, IJavaScriptResponse response) { response.addJavaScript("A|alert('Correct');A();"); } }); } }; add(correctexpression); but then generated JavaScript will be INFO: <?xml version="1.0" encoding="UTF-8"?><ajax-response><evaluate><![CDATA[(function() {A|alert('Correct');A();})();]]></evaluate><priority-evaluate><![CDATA[(function(){A|alert('Correct');A();} )();]]></priority-evaluate><evaluate><![CDATA[(function() {A|alert('Correct');A();})();]]></evaluate></ajax-response> ERROR: Wicket.Ajax.Call.processEvaluation: Exception evaluating javascript: ReferenceError: A is not defined, text: (function(){A|alert('Correct');A();} )(); ERROR: Wicket.Ajax.Call.processEvaluation: Exception evaluating javascript: ReferenceError: A is not defined, text: (function() {A|alert('Correct');A();})(); ERROR: Wicket.Ajax.Call.processEvaluation: Exception evaluating javascript: ReferenceError: A is not defined, text: (function(){A|alert('Correct');A();} )(); INFO: Response processed successfully. INFO: refocus last focused component not needed/allowed INFO: focus removed from correctexpression2 INFO: focus set on wicketDebugLink INFO: focus removed from wicketDebugLink Then because of the "anonymous" function (function() { ...} )(); around expression it will never match regular expression. So, feature A|...A(); is not really supported on 6.x
          Hide
          Ernesto Reinaldo Barreiro added a comment -

          wickstart

          Show
          Ernesto Reinaldo Barreiro added a comment - wickstart
          Ernesto Reinaldo Barreiro made changes -
          Attachment wicket5039.tar.gz [ 12569793 ]
          Hide
          Sven Meier added a comment -

          Thanks Ernesto for analysing this. I wonder what this feature was used for, but we should track this in a new issue.

          Show
          Sven Meier added a comment - Thanks Ernesto for analysing this. I wonder what this feature was used for, but we should track this in a new issue.
          Hide
          Martin Grigorov added a comment -

          I also don't know the roots of something|code feature. I've just migrated it from 1.5.x and even added JS tests for it.

          And yes, it seems I broke it with the introduction of the anonymous function which is used to minimize the number of eval's to 1 and to keep the variables local for each prepend/append script.

          Show
          Martin Grigorov added a comment - I also don't know the roots of something|code feature. I've just migrated it from 1.5.x and even added JS tests for it. And yes, it seems I broke it with the introduction of the anonymous function which is used to minimize the number of eval's to 1 and to keep the variables local for each prepend/append script.
          Hide
          Ernesto Reinaldo Barreiro added a comment -

          Sven.

          It is a pity that no comment was put on code about why this special case was needed... or a reference to a JIRA issue. Maybe someone can remember why was this introduced?

          I guess "original problem" is also present on 1.5.x... I will try to test that later on today.

          Show
          Ernesto Reinaldo Barreiro added a comment - Sven. It is a pity that no comment was put on code about why this special case was needed... or a reference to a JIRA issue. Maybe someone can remember why was this introduced? I guess "original problem" is also present on 1.5.x... I will try to test that later on today.
          Hide
          Didier Villevalois added a comment -

          Hi,

          I did not see this issue and commented on the issue that broke the use of identifier|jscode as permited by wicket-ajax-jquery.js (see https://issues.apache.org/jira/browse/WICKET-4881)

          Let me give an explanation of how I use this feature:

          Lets say you have a open modal window (ala bootstrap) you want to close it, change its content and reopen it in a single ajax request. For that you need to delay the component replacement until the javascript effect that closes the modal window. That animation is asynchronous, so the only way to do that is to register for the event triggered when the modal is effectively closed and call the notify() callback so that FunctionsExecuter can continue its processing.

          The identifier|jscode feature can not be called anymore since https://github.com/apache/wicket/commit/cbf76d0e8c355876e302699c0975971773941ae9 as the code is concatenated and each evaluation encapsulated in a '(function()

          { jscode })();' statement.

          A good solution for me would be:

          1) Introduce a protected JavaScriptFragment object in AbstractAjaxResponse

          protected class JavaScriptFragment { private String notifyCallbackName; private String script; // + constructors and getters }

          2) Provide new APIs on IJavascriptResponse:

          void prependJavaScript(String script);
          void prependJavaScript(String notifyCallbackName, String script);
          void appendJavaScript(String script);
          void appendJavaScript(String notifyCallbackName, String script);

          and deprecate addJavascript(..) as this is not very descriptive of where the script goes in the response (also its current implementation does not take profit of script concatenation).

          3) Make AbstractAjaxResponse collect JavaScriptFragments and decide itself what it can concatenate (it is not the responsability of AjaxXmlResponse to do so is a json-based response would have the same problems with browser-specific limited call stacks). We can concatenate scripts without callback use but can't concatenate scripts with explicit callbacks, right ?

          4) Make AbstractAjaxResponse define
          protected abstract void writePriorityEvaluation(Response response, String script, String callbackName (may be null))
          protected abstract void writeNormalEvaluation(Response response, String script, String callbackName (may be null))

          and use that in writeTo(..) to pass concatenated scripts or individual script with callback name.

          What do you think ? We could also (not disjunctive with the solution above) introduce additional APIs in wicket-ajax-jquery.js processEvaluation() to handle something like this:

          <[priority-]evaluate>
          function(scripts) {
          scripts.queueAsync(function() { jscode }

          );
          scripts.queueAsync(function()

          { jscode });
          scripts.queueSync(function(notify) { jscode that calls notify });
          scripts.queueAsync(function() { jscode }

          );
          scripts.queueAsync(function()

          { jscode }

          );
          // ....
          }
          </[priority-]evaluate>

          Such a queue would then be processed serially avoided the IE problem and still providing the possibility to delay component replacement.

          Any thoughts ?

          Show
          Didier Villevalois added a comment - Hi, I did not see this issue and commented on the issue that broke the use of identifier|jscode as permited by wicket-ajax-jquery.js (see https://issues.apache.org/jira/browse/WICKET-4881 ) Let me give an explanation of how I use this feature: Lets say you have a open modal window (ala bootstrap) you want to close it, change its content and reopen it in a single ajax request. For that you need to delay the component replacement until the javascript effect that closes the modal window. That animation is asynchronous, so the only way to do that is to register for the event triggered when the modal is effectively closed and call the notify() callback so that FunctionsExecuter can continue its processing. The identifier|jscode feature can not be called anymore since https://github.com/apache/wicket/commit/cbf76d0e8c355876e302699c0975971773941ae9 as the code is concatenated and each evaluation encapsulated in a '(function() { jscode })();' statement. A good solution for me would be: 1) Introduce a protected JavaScriptFragment object in AbstractAjaxResponse protected class JavaScriptFragment { private String notifyCallbackName; private String script; // + constructors and getters } 2) Provide new APIs on IJavascriptResponse: void prependJavaScript(String script); void prependJavaScript(String notifyCallbackName, String script); void appendJavaScript(String script); void appendJavaScript(String notifyCallbackName, String script); and deprecate addJavascript(..) as this is not very descriptive of where the script goes in the response (also its current implementation does not take profit of script concatenation). 3) Make AbstractAjaxResponse collect JavaScriptFragments and decide itself what it can concatenate (it is not the responsability of AjaxXmlResponse to do so is a json-based response would have the same problems with browser-specific limited call stacks). We can concatenate scripts without callback use but can't concatenate scripts with explicit callbacks, right ? 4) Make AbstractAjaxResponse define protected abstract void writePriorityEvaluation(Response response, String script, String callbackName (may be null)) protected abstract void writeNormalEvaluation(Response response, String script, String callbackName (may be null)) and use that in writeTo(..) to pass concatenated scripts or individual script with callback name. What do you think ? We could also (not disjunctive with the solution above) introduce additional APIs in wicket-ajax-jquery.js processEvaluation() to handle something like this: < [priority-] evaluate> function(scripts) { scripts.queueAsync(function() { jscode } ); scripts.queueAsync(function() { jscode }); scripts.queueSync(function(notify) { jscode that calls notify }); scripts.queueAsync(function() { jscode } ); scripts.queueAsync(function() { jscode } ); // .... } </ [priority-] evaluate> Such a queue would then be processed serially avoided the IE problem and still providing the possibility to delay component replacement. Any thoughts ?
          Hide
          Didier Villevalois added a comment -

          We could also revert the concatenation stuff and enhance the processing of evaluations in wicket-ajax-jquery.js so that it can concatenate several evaluations that do not use notify callbacks at runtime. Also a good thing would be to not use a regexp but use an attribute to convey the notify callback name.

          Show
          Didier Villevalois added a comment - We could also revert the concatenation stuff and enhance the processing of evaluations in wicket-ajax-jquery.js so that it can concatenate several evaluations that do not use notify callbacks at runtime. Also a good thing would be to not use a regexp but use an attribute to convey the notify callback name.
          Hide
          Martin Grigorov added a comment -

          I think Ernesto's patch should not be applied.
          Wicket gives the control to the application code by giving it the 'notify' function as argument. The application code should assure that it is called. I.e. it should wrap its code in try/finally.

          The problem with the broken functionality in Wicket 6.x will be addressed soon.

          Show
          Martin Grigorov added a comment - I think Ernesto's patch should not be applied. Wicket gives the control to the application code by giving it the 'notify' function as argument. The application code should assure that it is called. I.e. it should wrap its code in try/finally. The problem with the broken functionality in Wicket 6.x will be addressed soon.
          Hide
          Ernesto Reinaldo Barreiro added a comment -

          Martin.

          Ok... Maybe what is missing is improving the JavaScript doc to state this directly...

          Show
          Ernesto Reinaldo Barreiro added a comment - Martin. Ok... Maybe what is missing is improving the JavaScript doc to state this directly...
          Martin Grigorov made changes -
          Assignee Martin Grigorov [ mgrigorov ]
          Martin Grigorov made changes -
          Link This issue is broken by WICKET-4881 [ WICKET-4881 ]
          Martin Grigorov made changes -
          Summary ajax post call handlers might not get called because of failure on AJAX processEvaluation function Manual invocation of FunctionsExecutor#notify() is broken
          Martin Grigorov made changes -
          Affects Version/s 6.4.0 [ 12323450 ]
          Affects Version/s 1.4.21 [ 12320171 ]
          Affects Version/s 1.5.8 [ 12321662 ]
          Affects Version/s 6.5.0 [ 12323540 ]
          Hide
          Martin Grigorov added a comment -

          The code is fixed and javadoc is added to AjaxRequestTarget #prepend and #appendJavaScript

          Show
          Martin Grigorov added a comment - The code is fixed and javadoc is added to AjaxRequestTarget #prepend and #appendJavaScript
          Martin Grigorov made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Fix Version/s 6.7.0 [ 12323964 ]
          Resolution Fixed [ 1 ]
          Hide
          Ernesto Reinaldo Barreiro added a comment -

          Martin,

          Thanks for fixing this! Just one comment. Would it make sense to put some more info on JavaScript so that future developers know why this is needed? Something like

          // test if the javascript is in form of identifier|code
          // if it is, we allow for letting the javascript decide when the rest of processing will continue
          // by invoking identifier();
          // This is used for the cases when control of AJAX processing want to be transferred to user's code.
          // E.g. Users want to execute an effect and after that effect finishes continue with AJAX processing.
          // see WICKET-5039 for more explanations.

          So, future developers will not have to loose time wondering why this "cryptic" feature is needed.

          Cheers,

          Ernesto

          Show
          Ernesto Reinaldo Barreiro added a comment - Martin, Thanks for fixing this! Just one comment. Would it make sense to put some more info on JavaScript so that future developers know why this is needed? Something like // test if the javascript is in form of identifier|code // if it is, we allow for letting the javascript decide when the rest of processing will continue // by invoking identifier(); // This is used for the cases when control of AJAX processing want to be transferred to user's code. // E.g. Users want to execute an effect and after that effect finishes continue with AJAX processing. // see WICKET-5039 for more explanations. So, future developers will not have to loose time wondering why this "cryptic" feature is needed. Cheers, Ernesto
          Hide
          Sven Meier added a comment -

          Couldn't we just let go of this strange "|" syntax?
          On the server side If #appendJavaScript()'s argument matches a function declaration, just leave it that way.
          In the browser If this function has a parameter, pass the notify method to it and let is trigger it.

          Show
          Sven Meier added a comment - Couldn't we just let go of this strange "|" syntax? On the server side If #appendJavaScript()'s argument matches a function declaration, just leave it that way. In the browser If this function has a parameter, pass the notify method to it and let is trigger it.
          Hide
          Martin Grigorov added a comment -

          @Ernesto: I've updated the comments in the JS code.

          @Sven: I agree that this will look nicer but since it is an API break we can do it as early as 7.0.
          And still the code in wicket-ajax-jquery.js will have to check for "function()" vs. "function(functionName)" and pass 'notify' only in the second case and call it in the first case.

          Show
          Martin Grigorov added a comment - @Ernesto: I've updated the comments in the JS code. @Sven: I agree that this will look nicer but since it is an API break we can do it as early as 7.0. And still the code in wicket-ajax-jquery.js will have to check for "function()" vs. "function(functionName)" and pass 'notify' only in the second case and call it in the first case.
          Hide
          Martin Grigorov added a comment -
          Show
          Martin Grigorov added a comment - A blog article with demo app: http://wicketinaction.com/2013/02/replace-components-with-animation/
          Hide
          Didier Villevalois added a comment -

          Martin, Thank you very much for your very fast action, that will help us a lot!

          If you'd like some ideas to enhance all this for 7.0, i'd say a must-have would be to optionally be able to manually trigger individual DOM replacements directly from the produced Javascript code. Having all the components replaced in one shot is often a problem when you write applications with lots dynamic effects.

          Show
          Didier Villevalois added a comment - Martin, Thank you very much for your very fast action, that will help us a lot! If you'd like some ideas to enhance all this for 7.0, i'd say a must-have would be to optionally be able to manually trigger individual DOM replacements directly from the produced Javascript code. Having all the components replaced in one shot is often a problem when you write applications with lots dynamic effects.
          Hide
          Martin Grigorov added a comment -

          Hi Didier,

          Can you share your ideas in the mailing lists ? With as much details as possible.
          If you are not subscribed please send them to mgrigorov at apache org and I'll send them on behalf of you.

          Or see https://cwiki.apache.org/confluence/display/WICKET/Ideas+for+Wicket+7.0#IdeasforWicket7.0-Maketransitionspossible%2FeasywhenreplacingcomponentsusingAjax%2FPush - it sounds like what you provide.

          Show
          Martin Grigorov added a comment - Hi Didier, Can you share your ideas in the mailing lists ? With as much details as possible. If you are not subscribed please send them to mgrigorov at apache org and I'll send them on behalf of you. Or see https://cwiki.apache.org/confluence/display/WICKET/Ideas+for+Wicket+7.0#IdeasforWicket7.0-Maketransitionspossible%2FeasywhenreplacingcomponentsusingAjax%2FPush - it sounds like what you provide.
          Tobias Haupt made changes -
          Link This issue is related to WICKET-5243 [ WICKET-5243 ]
          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Resolved Resolved
          8d 1h 52m 1 Martin Grigorov 21/Feb/13 14:20

            People

            • Assignee:
              Martin Grigorov
              Reporter:
              Ernesto Reinaldo Barreiro
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development