ICEfaces
  1. ICEfaces
  2. ICE-9348

ACE Renderers have thread unsafe instance field

          Details

          • Type: Bug Bug
          • Status: Closed
          • Priority: Major Major
          • Resolution: Fixed
          • Affects Version/s: 3.3
          • Fix Version/s: EE-3.3.0.GA, 4.0.BETA, 4.0
          • Component/s: ACE-Components
          • Labels:
            None
          • Environment:
            ACE

            Description

            As reported in the forum, these renderers use instance fields that they read from and write to during rendering:

            - TextAreaEntryRenderer
            - SliderEntryRenderer
            - PanelRenderer
            - DateTimeEntryRenderer
            - CheckboxButtonRenderer
            - CellEditorRenderer

            A common pattern found in some of them is to use this code:

            public class DateTimeEntryRenderer extends InputRenderer {
                private Map<String, Object> domUpdateMap = new HashMap<String, Object>();

            Which involves the encode methods using the domUpdateMap to create a hashcode. The domUpdateMap should be declared inside the methods, which might involve re-arranging code between the encodeBegin and encodeEnd methods.

            Renderers are application scoped, and can be used simultaneously by different requests / lifecycles. They should never use instance fields that are written to and then read from during a discrete render, since the same Renderer instance is used across threads for different views and users.

            We'll need to audit all of our ACE Renderers, as the forum posting might not have been comprehensive.

              Activity

              Ascending order - Click to sort in descending order
              Mark Collette created issue -
              Mark Collette made changes -
              Field Original Value New Value
              Description As reported in the forum, these renderers

              - TextAreaEntryRenderer
              - SliderEntryRenderer
              - PanelRenderer
              - DateTimeEntryRenderer
              - CheckboxButtonRenderer
              - CellEditorRenderer

              Use this code:

              public class DateTimeEntryRenderer extends InputRenderer {
                  private Map<String, Object> domUpdateMap = new HashMap<String, Object>();

              Which involves the encode methods using the domUpdateMap to create a hashcode. The domUpdateMap should be declared inside the methods, which might involve re-arranging code between the encodeBegin and encodeEnd methods.

              Renderers are application scoped, and can be used simultaneously by different requests / lifecycles. They should never use instance fields.
              		As reported in the forum, these renderers

              - TextAreaEntryRenderer
              - SliderEntryRenderer
              - PanelRenderer
              - DateTimeEntryRenderer
              - CheckboxButtonRenderer
              - CellEditorRenderer

              Use this code:

              public class DateTimeEntryRenderer extends InputRenderer {
                  private Map<String, Object> domUpdateMap = new HashMap<String, Object>();

              Which involves the encode methods using the domUpdateMap to create a hashcode. The domUpdateMap should be declared inside the methods, which might involve re-arranging code between the encodeBegin and encodeEnd methods.

              Renderers are application scoped, and can be used simultaneously by different requests / lifecycles. They should never use instance fields.

              We'll need to audit all of our ACE Renderers, as the forum posting might not have been comprehensive.
              Repository Revision Date User Message
              ICEsoft Public SVN Repository #36189 Wed Jun 12 15:49:17 MDT 2013 nils.lundquist ICE-9348 - Removed CellEditorRenderer state.
              Files Changed
              Commit graph MODIFY /icefaces3/trunk/icefaces/ace/component/src/org/icefaces/ace/component/celleditor/CellEditorRenderer.java
              Hide
              Permalink
              Nils Lundquist added a comment -

              Revision #36189
              Committed by nils.lundquist
              3 minutes ago ICE-9348 - Removed CellEditorRenderer state.

              Show
              Nils Lundquist added a comment - Revision #36189 Committed by nils.lundquist 3 minutes ago ICE-9348 - Removed CellEditorRenderer state.
              Hide
              Permalink
              Mark Collette added a comment -

              Also, MessageRenderer and MessagesRenderer use:

              private Logger logger = Logger.getLogger(this.getClass().getName());

              whereas the rest of our code uses static Logger instances. This shouldn't be an issue though.

              Show
              Mark Collette added a comment - Also, MessageRenderer and MessagesRenderer use: private Logger logger = Logger.getLogger(this.getClass().getName()); whereas the rest of our code uses static Logger instances. This shouldn't be an issue though.
              Hide
              Permalink
              yip.ng added a comment -

              This was done on purpose to avoid hard coding the logger name. (See the use of this?) I have seen too many cases where the line is just copied and pasted without changing the logger name, leading to misleading log messages.

              Show
              yip.ng added a comment - This was done on purpose to avoid hard coding the logger name. (See the use of this ?) I have seen too many cases where the line is just copied and pasted without changing the logger name, leading to misleading log messages.
              Repository Revision Date User Message
              ICEsoft Public SVN Repository #36194 Wed Jun 12 17:06:33 MDT 2013 yip.ng ICE-9348: ACE Renderers have thread unsafe instance field.
              Files Changed
              Commit graph MODIFY /icefaces3/trunk/icefaces/ace/component/src/org/icefaces/ace/component/datetimeentry/DateTimeEntryRenderer.java
              Hide
              Permalink
              yip.ng added a comment -

              DateTimeEntryRenderer done.

              M: C:\svn\ossrepo\icefaces3\trunk\icefaces\ace\component\src\org\icefaces\ace\component\datetimeentry\DateTimeEntryRenderer.java#36194

              M: http:\\10.18.39.25:8888\svn\ossrepo\icefaces-ee\tags\icefaces-ee-3.3.0.GA\icefaces\ace\component\src\org\icefaces\ace\component\datetimeentry\DateTimeEntryRenderer.java#36195

              Show
              yip.ng added a comment - DateTimeEntryRenderer done. M: C:\svn\ossrepo\icefaces3\trunk\icefaces\ace\component\src\org\icefaces\ace\component\datetimeentry\DateTimeEntryRenderer.java#36194 M: http:\\10.18.39.25:8888\svn\ossrepo\icefaces-ee\tags\icefaces-ee-3.3.0.GA\icefaces\ace\component\src\org\icefaces\ace\component\datetimeentry\DateTimeEntryRenderer.java#36195
              Repository Revision Date User Message
              ICEsoft Public SVN Repository #36201 Wed Jun 12 17:49:21 MDT 2013 art.zambrano ICE-9348 ACE Renderers have thread unsafe instance field
              Files Changed
              Commit graph MODIFY /icefaces3/trunk/icefaces/ace/component/src/org/icefaces/ace/component/panel/PanelRenderer.java
              Hide
              Permalink
              Arturo Zambrano added a comment -

              Committed fix for PanelRenderer.java at revision 36201 to the trunk and at revision 36202 to the 3.3 EE tag.

              Show
              Arturo Zambrano added a comment - Committed fix for PanelRenderer.java at revision 36201 to the trunk and at revision 36202 to the 3.3 EE tag.
              Repository Revision Date User Message
              ICEsoft Public SVN Repository #36203 Wed Jun 12 17:55:09 MDT 2013 yip.ng ICE-9348: ACE Renderers have thread unsafe instance field.
              Files Changed
              Commit graph MODIFY /icefaces3/trunk/icefaces/ace/component/src/org/icefaces/ace/component/textareaentry/TextAreaEntryRenderer.java
              Commit graph MODIFY /icefaces3/trunk/icefaces/ace/component/src/org/icefaces/ace/renderkit/InputRenderer.java
              Hide
              Permalink
              yip.ng added a comment -

              TextAreaEntryRenderer done.

              M: C:\svn\ossrepo\icefaces3\trunk\icefaces\ace\component\src\org\icefaces\ace\renderkit\InputRenderer.java#36203
              M: C:\svn\ossrepo\icefaces3\trunk\icefaces\ace\component\src\org\icefaces\ace\component\textareaentry\TextAreaEntryRenderer.java#36203

              M: http:\\10.18.39.25:8888\svn\ossrepo\icefaces-ee\tags\icefaces-ee-3.3.0.GA\icefaces\ace\component\src\org\icefaces\ace\renderkit\InputRenderer.java#36204
              M: http:\\10.18.39.25:8888\svn\ossrepo\icefaces-ee\tags\icefaces-ee-3.3.0.GA\icefaces\ace\component\src\org\icefaces\ace\component\textareaentry\TextAreaEntryRenderer.java#36204

              Show
              yip.ng added a comment - TextAreaEntryRenderer done. M: C:\svn\ossrepo\icefaces3\trunk\icefaces\ace\component\src\org\icefaces\ace\renderkit\InputRenderer.java#36203 M: C:\svn\ossrepo\icefaces3\trunk\icefaces\ace\component\src\org\icefaces\ace\component\textareaentry\TextAreaEntryRenderer.java#36203 M: http:\\10.18.39.25:8888\svn\ossrepo\icefaces-ee\tags\icefaces-ee-3.3.0.GA\icefaces\ace\component\src\org\icefaces\ace\renderkit\InputRenderer.java#36204 M: http:\\10.18.39.25:8888\svn\ossrepo\icefaces-ee\tags\icefaces-ee-3.3.0.GA\icefaces\ace\component\src\org\icefaces\ace\component\textareaentry\TextAreaEntryRenderer.java#36204
              Repository Revision Date User Message
              ICEsoft Public SVN Repository #36217 Thu Jun 13 12:37:22 MDT 2013 art.zambrano ICE-9348 ACE Renderers have thread unsafe instance field
              Files Changed
              Commit graph MODIFY /icefaces3/trunk/icefaces/ace/component/src/org/icefaces/ace/component/sliderentry/SliderEntryRenderer.java
              Repository Revision Date User Message
              ICEsoft Public SVN Repository #36218 Thu Jun 13 13:42:40 MDT 2013 yip.ng ICE-9348: ACE Renderers have thread unsafe instance field.
              Files Changed
              Commit graph MODIFY /icefaces3/trunk/icefaces/ace/component/src/org/icefaces/ace/component/message/MessageRenderer.java
              Commit graph MODIFY /icefaces3/trunk/icefaces/ace/component/src/org/icefaces/ace/component/growlmessages/GrowlMessagesRenderer.java
              Commit graph MODIFY /icefaces3/trunk/icefaces/ace/component/src/org/icefaces/ace/component/growlmessages/GrowlMessagesMeta.java
              Commit graph MODIFY /icefaces3/trunk/icefaces/ace/component/src/org/icefaces/ace/component/messages/MessagesRenderer.java
              Hide
              Permalink
              yip.ng added a comment -

              Message renderers done.

              M: C:\svn\ossrepo\icefaces3\trunk\icefaces\ace\component\src\org\icefaces\ace\component\growlmessages\GrowlMessagesMeta.java#36218
              M: C:\svn\ossrepo\icefaces3\trunk\icefaces\ace\component\src\org\icefaces\ace\component\growlmessages\GrowlMessagesRenderer.java#36218
              M: C:\svn\ossrepo\icefaces3\trunk\icefaces\ace\component\src\org\icefaces\ace\component\message\MessageRenderer.java#36218
              M: C:\svn\ossrepo\icefaces3\trunk\icefaces\ace\component\src\org\icefaces\ace\component\messages\MessagesRenderer.java#36218

              M: http:\\10.18.39.25:8888\svn\ossrepo\icefaces-ee\tags\icefaces-ee-3.3.0.GA\icefaces\ace\component\src\org\icefaces\ace\component\message\MessageRenderer.java#36219
              M: http:\\10.18.39.25:8888\svn\ossrepo\icefaces-ee\tags\icefaces-ee-3.3.0.GA\icefaces\ace\component\src\org\icefaces\ace\component\messages\MessagesRenderer.java#36219

              Show
              yip.ng added a comment - Message renderers done. M: C:\svn\ossrepo\icefaces3\trunk\icefaces\ace\component\src\org\icefaces\ace\component\growlmessages\GrowlMessagesMeta.java#36218 M: C:\svn\ossrepo\icefaces3\trunk\icefaces\ace\component\src\org\icefaces\ace\component\growlmessages\GrowlMessagesRenderer.java#36218 M: C:\svn\ossrepo\icefaces3\trunk\icefaces\ace\component\src\org\icefaces\ace\component\message\MessageRenderer.java#36218 M: C:\svn\ossrepo\icefaces3\trunk\icefaces\ace\component\src\org\icefaces\ace\component\messages\MessagesRenderer.java#36218 M: http:\\10.18.39.25:8888\svn\ossrepo\icefaces-ee\tags\icefaces-ee-3.3.0.GA\icefaces\ace\component\src\org\icefaces\ace\component\message\MessageRenderer.java#36219 M: http:\\10.18.39.25:8888\svn\ossrepo\icefaces-ee\tags\icefaces-ee-3.3.0.GA\icefaces\ace\component\src\org\icefaces\ace\component\messages\MessagesRenderer.java#36219
              Hide
              Permalink
              Arturo Zambrano added a comment -

              Committed fix for SliderEntryRenderer.java at revision 36217 to the trunk and at revision 36220 to the 3.3 EE tag.

              Show
              Arturo Zambrano added a comment - Committed fix for SliderEntryRenderer.java at revision 36217 to the trunk and at revision 36220 to the 3.3 EE tag.
              Mark Collette made changes -
              Description As reported in the forum, these renderers

              - TextAreaEntryRenderer
              - SliderEntryRenderer
              - PanelRenderer
              - DateTimeEntryRenderer
              - CheckboxButtonRenderer
              - CellEditorRenderer

              Use this code:

              public class DateTimeEntryRenderer extends InputRenderer {
                  private Map<String, Object> domUpdateMap = new HashMap<String, Object>();

              Which involves the encode methods using the domUpdateMap to create a hashcode. The domUpdateMap should be declared inside the methods, which might involve re-arranging code between the encodeBegin and encodeEnd methods.

              Renderers are application scoped, and can be used simultaneously by different requests / lifecycles. They should never use instance fields.

              We'll need to audit all of our ACE Renderers, as the forum posting might not have been comprehensive.               		As reported in the forum, these renderers use instance fields that they read from and write to during rendering:

              - TextAreaEntryRenderer
              - SliderEntryRenderer
              - PanelRenderer
              - DateTimeEntryRenderer
              - CheckboxButtonRenderer
              - CellEditorRenderer

              A common pattern found in some of them is to use this code:

              public class DateTimeEntryRenderer extends InputRenderer {
                  private Map<String, Object> domUpdateMap = new HashMap<String, Object>();

              Which involves the encode methods using the domUpdateMap to create a hashcode. The domUpdateMap should be declared inside the methods, which might involve re-arranging code between the encodeBegin and encodeEnd methods.

              Renderers are application scoped, and can be used simultaneously by different requests / lifecycles. They should never use instance fields that are written to and then read from during a discrete render, since the same Renderer instance is used across threads for different views and users.

              We'll need to audit all of our ACE Renderers, as the forum posting might not have been comprehensive.
              Mark Collette made changes -
              Assignee Priority P1 [ 10010 ]
              Mark Collette made changes -
              Assignee Ken Fyten [ ken.fyten ]
              Hide
              Permalink
              Mark Collette added a comment - - edited

              Due to the new automated test in ICE-9358 that is overly stringent, many renderers that are not broken needed to be modified to satisfy the test.

              icefaces3 trunk
              Subversion 36236,36238.

              icefaces EE 3.3 branch tag
              Subversion 36241

              Show
              Mark Collette added a comment - - edited Due to the new automated test in ICE-9358 that is overly stringent, many renderers that are not broken needed to be modified to satisfy the test. icefaces3 trunk Subversion 36236,36238. icefaces EE 3.3 branch tag Subversion 36241
              Repository Revision Date User Message
              ICEsoft Public SVN Repository #36236 Fri Jun 14 15:02:07 MDT 2013 mark.collette ICE-9348 : ACE Renderers have thread unsafe instance field
              Files Changed
              Commit graph MODIFY /icefaces3/trunk/icefaces/ace/component/src/org/icefaces/ace/component/fileentry/FileEntryRenderer.java
              Commit graph MODIFY /icefaces3/trunk/icefaces/ace/component/src/org/icefaces/ace/component/linkbutton/LinkButtonRenderer.java
              Commit graph MODIFY /icefaces3/trunk/icefaces/ace/component/src/org/icefaces/ace/component/datatable/DataTableRenderer.java
              Commit graph MODIFY /icefaces3/trunk/icefaces/ace/component/src/org/icefaces/ace/component/tabset/TabSetRenderer.java
              Commit graph MODIFY /icefaces3/trunk/icefaces/ace/component/src/org/icefaces/ace/component/roweditor/RowEditorRenderer.java
              Repository Revision Date User Message
              ICEsoft Public SVN Repository #36238 Fri Jun 14 15:12:16 MDT 2013 mark.collette ICE-9348 : ACE Renderers have thread unsafe instance field
              Files Changed
              Commit graph MODIFY /icefaces3/trunk/icefaces/core/src/main/java/org/icefaces/impl/renderkit/html_basic/SingleSubmitRenderer.java
              Repository Revision Date User Message
              ICEsoft Public SVN Repository #36254 Mon Jun 17 11:18:54 MDT 2013 nils.lundquist ICE-9348 - Changed use of $.attr('value') for $ val()
              Files Changed
              Commit graph MODIFY /icefaces3/trunk/icefaces/ace/component/resources/icefaces.ace/list/list.js
              Hide
              Permalink
              Ken Fyten added a comment -

              Single remaining issue is the ace:checkboxButton renderer, assigned to Nils:

              WARNING: For class org.icefaces.ace.component.checkboxbutton.CheckboxButtonRenderer, the field 'uiParamChildren' is not declared 'static final'. Renderer classes must not have instance fields, and must be re-entrant.

              Show
              Ken Fyten added a comment - Single remaining issue is the ace:checkboxButton renderer, assigned to Nils: WARNING: For class org.icefaces.ace.component.checkboxbutton.CheckboxButtonRenderer, the field 'uiParamChildren' is not declared 'static final'. Renderer classes must not have instance fields, and must be re-entrant.
              Ken Fyten made changes -
              Assignee Ken Fyten [ ken.fyten ] Nils Lundquist [ nils.lundquist ]
              Affects Version/s 3.3 [ 10370 ]
              Hide
              Permalink
              Nils Lundquist added a comment -

              Revision #36260
              Committed by nils.lundquist
              Moments ago
              ICE-9348 - Changed useless uiParamChildren variable placement.

              Revision #36261
              Committed by nils.lundquist
              Moments ago
              ICE-9348 - Changed useless uiParamChildren variable placement

              Show
              Nils Lundquist added a comment - Revision #36260 Committed by nils.lundquist Moments ago ICE-9348 - Changed useless uiParamChildren variable placement. Revision #36261 Committed by nils.lundquist Moments ago ICE-9348 - Changed useless uiParamChildren variable placement
              Nils Lundquist made changes -
              Status Open [ 1 ] Resolved [ 5 ]
              Resolution Fixed [ 1 ]
              Repository Revision Date User Message
              ICEsoft Public SVN Repository #36260 Mon Jun 17 14:12:42 MDT 2013 nils.lundquist ICE-9348 - Changed useless uiParamChildren variable placement.
              Files Changed
              Commit graph MODIFY /icefaces3/trunk/icefaces/ace/component/src/org/icefaces/ace/component/checkboxbutton/CheckboxButtonRenderer.java
              Ken Fyten made changes -
              Fix Version/s 4.0 [ 11382 ]
              Ken Fyten made changes -
              Status Resolved [ 5 ] Closed [ 6 ]

                People

                • Assignee:
                  Nils Lundquist
                  Reporter:
                  Mark Collette
                • Votes:
                  0 Vote for this issue
                  Watchers:
                  4 Start watching this issue

                  Dates

                  • Created:
                    Updated:
                    Resolved: