Uploaded image for project: 'Sakai'
  1. Sakai
  2. SAK-13679

Generic Jira: Mutable servlet field [Static Code Review]

    XMLWordPrintable

    Details

      Description

      Generic Jira for mutable servlet fields

      I'm seeing a pattern that looks very broken.

      It consists of a m_ready servlet field and a inner Thread that sets that field without synchronization. It is possible that the thread state never gets communicated to main memory, or it might be communicated too early(signaling readiness while in fact the init method is still running).

      What strikes me as odd is that the Servlet spec specifies that the init method is called only once(1) in the lifetime of a serlvet and that service will not be called before init has finished. This construct seems to think otherwise. If this is a workaround for the container it should be documented, in any other case it is wrong.

      If lazy initialization is needed java.util.concurrent.Future is a much cleaner way to implement that. Other developers immediately see what's going on and do not have to try to grasp complex,dead,incorrect code.

      I have seen it in org/sakaiproject/access/tool/AccessServlet and org/sakaiproject/citation/servlet/CitationServlet

      see http://qa1-nl.sakaiproject.org/codereview/bug_dashboard/findbugs_MT_CORRECTNESS.html

      http://qa1-nl.sakaiproject.org/codereview/trunk/api/org/sakaiproject/access/tool/AccessServlet.java.html#130
      http://qa1-nl.sakaiproject.org/codereview/trunk/api/org/sakaiproject/citation/servlet/CitationServlet.java.html#119
      http://qa1-nl.sakaiproject.org/codereview/trunk/api/org/sakaiproject/citation/servlet/CitationServlet.java.html#370
      http://qa1-nl.sakaiproject.org/codereview/trunk/api/org/sakaiproject/entitybroker/servlet/DirectServlet.java.html#115
      http://qa1-nl.sakaiproject.org/codereview/trunk/api/org/sakaiproject/entitybroker/servlet/DirectServlet.java.html#142
      http://qa1-nl.sakaiproject.org/codereview/trunk/api/org/sakaiproject/entitybroker/servlet/DirectServlet.java.html#160
      http://qa1-nl.sakaiproject.org/codereview/trunk/api/org/sakaiproject/james/JamesServlet.java.html#76
      http://qa1-nl.sakaiproject.org/codereview/trunk/api/org/sakaiproject/james/JamesServlet.java.html#130
      http://qa1-nl.sakaiproject.org/codereview/trunk/api/org/sakaiproject/search/tool/ControllerServlet.java.html#112
      http://qa1-nl.sakaiproject.org/codereview/trunk/api/org/sakaiproject/search/tool/ControllerServlet.java.html#126
      http://qa1-nl.sakaiproject.org/codereview/trunk/api/org/sakaiproject/search/tool/ControllerServlet.java.html#133
      http://qa1-nl.sakaiproject.org/codereview/trunk/api/org/sakaiproject/tool/help/ContentServlet.java.html#114
      http://qa1-nl.sakaiproject.org/codereview/trunk/api/org/sakaiproject/tool/podcasts/RSSPodfeedServlet.java.html#229
      http://qa1-nl.sakaiproject.org/codereview/trunk/api/uk/ac/cam/caret/sakai/rwiki/tool/RWikiServlet.java.html#138

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

                People

                • Assignee:
                  Unassigned
                  Reporter:
                  gnooteb2 Gilgamesh Nootebos (Inactive)
                • Votes:
                  1 Vote for this issue
                  Watchers:
                  2 Start watching this issue

                  Dates

                  • Created:
                    Updated:
                    Resolved:

                    Git Source Code