You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@wicket.apache.org by "Joonas Koivunen (JIRA)" <ji...@apache.org> on 2011/03/28 18:10:06 UTC

[jira] [Created] (WICKET-3564) AjaxRequestTarget#addComponent(..) should guard against improper usage

AjaxRequestTarget#addComponent(..) should guard against improper usage
----------------------------------------------------------------------

                 Key: WICKET-3564
                 URL: https://issues.apache.org/jira/browse/WICKET-3564
             Project: Wicket
          Issue Type: Improvement
          Components: wicket-core
    Affects Versions: 1.4.17
            Reporter: Joonas Koivunen


It's possible to call ART#addComponent(...) from (for example) Component#onComponentTag(..) which will later throw ConcurrentModificationException from line 675 (getting next from iterator).

Guarding the addComponent(Component, String) and the other add* and register* methods would seem better in the terms of fail-fast behaviour and save someone else a bit of debugging time.

Patching this is rather trivial at least for Components; for javascripts I'm not sure if correctly blocking adding prependJs just before rendering it could break someones (already broken, unrendered) javascript?

--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira

[jira] [Commented] (WICKET-3564) AjaxRequestTarget#addComponent(..) should guard against improper usage

Posted by "Igor Vaynberg (JIRA)" <ji...@apache.org>.

    [ https://issues.apache.org/jira/browse/WICKET-3564?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13052604#comment-13052604 ] 

Igor Vaynberg commented on WICKET-3564:
---------------------------------------

Joonas, that would be great.

> AjaxRequestTarget#addComponent(..) should guard against improper usage
> ----------------------------------------------------------------------
>
>                 Key: WICKET-3564
>                 URL: https://issues.apache.org/jira/browse/WICKET-3564
>             Project: Wicket
>          Issue Type: Improvement
>          Components: wicket-core
>    Affects Versions: 1.4.17
>            Reporter: Joonas Koivunen
>            Assignee: Juergen Donnerstag
>             Fix For: 1.5-RC5
>
>         Attachments: 3564-list_comp_resp.diff
>
>
> It's possible to call ART#addComponent(...) from (for example) Component#onComponentTag(..) which will later throw ConcurrentModificationException from line 675 (getting next from iterator).
> Guarding the addComponent(Component, String) and the other add* and register* methods would seem better in the terms of fail-fast behaviour and save someone else a bit of debugging time.
> Patching this is rather trivial at least for Components; for javascripts I'm not sure if correctly blocking adding prependJs just before rendering it could break someones (already broken, unrendered) javascript?

--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira

[jira] [Resolved] (WICKET-3564) AjaxRequestTarget#addComponent(..) should guard against improper usage

Posted by "Juergen Donnerstag (JIRA)" <ji...@apache.org>.

     [ https://issues.apache.org/jira/browse/WICKET-3564?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Juergen Donnerstag resolved WICKET-3564.
----------------------------------------

       Resolution: Fixed
    Fix Version/s:     (was: 1.5-RC5)
                   1.5-RC6

patch applied

> AjaxRequestTarget#addComponent(..) should guard against improper usage
> ----------------------------------------------------------------------
>
>                 Key: WICKET-3564
>                 URL: https://issues.apache.org/jira/browse/WICKET-3564
>             Project: Wicket
>          Issue Type: Improvement
>          Components: wicket-core
>    Affects Versions: 1.4.17
>            Reporter: Joonas Koivunen
>            Assignee: Juergen Donnerstag
>             Fix For: 1.5-RC6
>
>         Attachments: 3564-list_comp_resp.diff, fix-3564-2.diff, wi-3564-quickstart.tar.gz
>
>
> It's possible to call ART#addComponent(...) from (for example) Component#onComponentTag(..) which will later throw ConcurrentModificationException from line 675 (getting next from iterator).
> Guarding the addComponent(Component, String) and the other add* and register* methods would seem better in the terms of fail-fast behaviour and save someone else a bit of debugging time.
> Patching this is rather trivial at least for Components; for javascripts I'm not sure if correctly blocking adding prependJs just before rendering it could break someones (already broken, unrendered) javascript?

--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira

[jira] [Commented] (WICKET-3564) AjaxRequestTarget#addComponent(..) should guard against improper usage

Posted by "Martijn Dashorst (JIRA)" <ji...@apache.org>.

    [ https://issues.apache.org/jira/browse/WICKET-3564?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13052545#comment-13052545 ] 

Martijn Dashorst commented on WICKET-3564:
------------------------------------------

The newly introduced state/assertions try to prevent
ConcurrentModificationExceptions when adding listeners to the target
while the listeners are being processed. While this is a laudable
goal, it fails when something that just works without causing
ConcurrentModificationExceptions now suddenly is forbidden.

That said, WiQuery does the following to ensure that a component does
the JQuery initialization dance. The javascript js is something like
"$('id').activatePlugin();"

   AjaxRequestTarget ajaxRequestTarget = (AjaxRequestTarget) requestHandler;
   ajaxRequestTarget.addListener(new IListener() {
       public void onAfterRespond(Map<String, Component> map,
               IJavaScriptResponse response) {
           response.addJavaScript(js);
       }

       public void onBeforeRespond(Map<String, Component> map,
AjaxRequestTarget target) {
           // Do nothing
       }
   });

This is (ultimately) called from AjaxRequestTarget#renderHead().

Now I am quite positive that this is a bit of over-engineering/left
overs from 1.4 days on WiQuery's part, according to me the following
should also work:

   AjaxRequestTarget ajaxRequestTarget = (AjaxRequestTarget) requestHandler;
   requestHandler.getHeaderResponse().renderOnDomReadyJavaScript(js);

Given that WiQuery probably does something stupid (the simplified code
suggests such), I do think that the current implementation of
restricting additions to the listeners is too strict.

There are a couple of things we can change:
 1. relax the assertions such that they only cover the cases where
things actually go wrong, or
 2. use CopyOnWriteArraySet instead of HashSet to make iteration safe
even when things get added to the set, or
 3. ...?

Number 2 sounds like a good idea, but might introduce errors where
someone is adding a listener which doesn't get processed since the
iterator works on a previous copy—hard to debug, but quite easy to
track (keep a previous size and compare/assert after the loop). It
also consumes more memory during request processing.

I agree with the issue that a ConcurrentModificationException is less
descriptive than asserting that things are set in stone. So I'd like
to opt for number 1 and relax the assertions to just those spots where
things go wrong, *and* properly document it instead of relying on
runtime exceptions that may/may not occur.


> AjaxRequestTarget#addComponent(..) should guard against improper usage
> ----------------------------------------------------------------------
>
>                 Key: WICKET-3564
>                 URL: https://issues.apache.org/jira/browse/WICKET-3564
>             Project: Wicket
>          Issue Type: Improvement
>          Components: wicket-core
>    Affects Versions: 1.4.17
>            Reporter: Joonas Koivunen
>            Assignee: Juergen Donnerstag
>             Fix For: 1.5-RC5
>
>         Attachments: 3564-list_comp_resp.diff
>
>
> It's possible to call ART#addComponent(...) from (for example) Component#onComponentTag(..) which will later throw ConcurrentModificationException from line 675 (getting next from iterator).
> Guarding the addComponent(Component, String) and the other add* and register* methods would seem better in the terms of fail-fast behaviour and save someone else a bit of debugging time.
> Patching this is rather trivial at least for Components; for javascripts I'm not sure if correctly blocking adding prependJs just before rendering it could break someones (already broken, unrendered) javascript?

--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira