glassfish
  1. glassfish
  2. GLASSFISH-18970

Potential atomicity violations and improvement in web_container component

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.1.2
    • Fix Version/s: 4.0_b50_ms4
    • Component/s: web_container
    • Labels:
      None

      Description

      My name is Yu Lin. I'm a Ph.D. student in the CS department at
      UIUC. I'm currently doing research on mining Java concurrent library
      misusages. I found some misuses of ConcurrentHashMap in web
      component of GlassFish 3.1.2, which may result in potential atomicity
      violation bugs or harm the performance.

      The code below is a snapshot of the code in file
      web/war-util/src/main/java/org/glassfish/web/loader/WebappClassLoader.java
      from line 1528 to 1529

      L1528 if ((pc = loaderPC.get(codeUrl)) == null) {
      L1529 pc = super.getPermissions(codeSource);
      L1530 if (pc != null) {
      L1531 Iterator<Permission> perms = permissionList.iterator();
      L1532 while (perms.hasNext())

      { L1533 Permission p = perms.next(); L1534 pc.add(p); L1535 }

      L1536 loaderPC.put(codeUrl,pc);
      L1537 }
      L1538 }
      L1539 return (pc);

      In the code above, there might be an atomicity violation: suppose a
      thread T1 executes line 1528 and finds out the concurrent hashmap
      "loaderPC" does not contain the key "codeUrl". Before it
      gets to execute line 1536, another thread T2 puts a pair <codeUrl, v>
      in the map "loaderPC". Now thread T1 resumes execution and
      it will overwrite the value written by thread T2. Thus, the code no
      longer preserves the "put-if-absent" semantics, and thread T2 will
      return a "pc" object that is not in the map "loaderPC". May this
      thread interleaving result in buggy behavior?

      Similar problem can be found in
      web/web-glue/src/main/java/com/sun/enterprise/web/ContextFacade.java
      line 316.

      Another possible atomicity vioaltion may also happen at line 1943 in
      web/web-core/src/main/java/org/apache/catalina/session/StandardSession.java:

      L1943 if (attributes == null)
      L1944 attributes = new ConcurrentHashMap<String, Object>();
      // some <key, value> pairs are put into "attributes"

      A possible buggy scenario is both threads T1 and T2 finds attributes
      is null at the same time (line 1943). After T1 creates a new empty map
      and puts some <key, value> pairs into "attributes", T2 also creates a
      empty map and writes to "attributes". Then the <key, value> pairs put
      by T1 will be lost. If this may happen, we have to add synchronization
      when initializing "attributes". However, I'm not sure whether this
      interleaving will happen.

      Meanwhile, for some cases, though there are no atomicity violations,
      we may improve the performance of the code by removing some
      unnecessary synchronizations:

      In
      web/web-glue/src/main/java/com/sun/enterprise/web/logger/FileLoggerHandlerFactory.java
      line 75, if we use "putIfAbsent" method, we can remove the
      "synchronized" on "getHandler" method. Also at line 81, the
      "synchronized" on "removeHandler" is unnecessary since the operations
      provided by ConcurrentHashMap is internally synchronized.

      Similarly, can we improve the
      web/web-core/src/main/java/org/apache/catalina/core/ApplicationContext.java
      by removing the synchronization at line 1029, using "putIfAbsent" at
      line 1035, and changing the type of "params" from "ArrayList" to a
      concurrent list "CopyOnWriteArrayList"?

      This issue is related to GLASSFISH-18964. I attach a patch that can fix the problems.

        Activity

        Hide
        Shing Wai Chan added a comment -

        The changes for WebappClassLoader and ContextFacade are good.

        For StandardSession, those logic is not an issue as it is for deserialization.

        For FileLoggerHandlerFactory, I prefer to keep the existing logic as the API is not called often and I have concern that it will create more FileLoggerHandler object that necessary.

        For ApplicationContext, the params is synchronized in different places. We need to have a more detailed analysis for this. Changing parameters to use ConcurrentHashMap will generate several FindBugs issues.

        For StandardContext, the change is not needed as the callers are synchronized. Using CopyOnWriteArrayList will add more overhead in copying.

        Sending war-util/src/main/java/org/glassfish/web/loader/WebappClassLoader.java
        Sending web-glue/src/main/java/com/sun/enterprise/web/ContextFacade.java
        Transmitting file data ..
        Committed revision 55279.

        Show
        Shing Wai Chan added a comment - The changes for WebappClassLoader and ContextFacade are good. For StandardSession, those logic is not an issue as it is for deserialization. For FileLoggerHandlerFactory, I prefer to keep the existing logic as the API is not called often and I have concern that it will create more FileLoggerHandler object that necessary. For ApplicationContext, the params is synchronized in different places. We need to have a more detailed analysis for this. Changing parameters to use ConcurrentHashMap will generate several FindBugs issues. For StandardContext, the change is not needed as the callers are synchronized. Using CopyOnWriteArrayList will add more overhead in copying. Sending war-util/src/main/java/org/glassfish/web/loader/WebappClassLoader.java Sending web-glue/src/main/java/com/sun/enterprise/web/ContextFacade.java Transmitting file data .. Committed revision 55279.
        Hide
        Shing Wai Chan added a comment -

        The fixed has been checkin to trunk as mentioned above.

        Show
        Shing Wai Chan added a comment - The fixed has been checkin to trunk as mentioned above.

          People

          • Assignee:
            Shing Wai Chan
            Reporter:
            jacklondongood
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: