Derby
  1. Derby
  2. DERBY-4723

Using an instance lock to protect static shared data in EmbedPooledConnection

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 10.5.1.1
    • Fix Version/s: 10.5.3.2, 10.6.2.4, 10.7.1.1
    • Component/s: JDBC
    • Labels:
      None

      Description

      EmbedPooledConnection has the unsafe synchronization as follow.

      private static int idCounter = 0;

      private synchronized int nextId()

      { return idCounter++; }

      idCounter is a static shared data, and it is not proper to use a instance lock to protect it, especially when two instance of the class are created.

      it would be more safer to write this instead:

      private static synchronized int nextId()
      { return idCounter++; }

        Issue Links

          Activity

          Hide
          Kristian Waagan added a comment -

          Hi Wendy,

          Thanks for reporting the issue.
          The proposed fix sounds good, but before we implement it we may want to verify that we indeed need this data to be shared.

          As far as I can see, the id is used only for tracing. Having a local counter together with the id for the real connection would uniquely identify a pooled connection (would have to save this, since the real connection can be nullified), but we would then loose creation ordering information.
          In any case I think accessing the shared state in this case won't cause any performance issues (once per pooled connection creation, minimal critical section size).

          Opinions anyone?
          If I don't hear anything, I'll commit Wendy's proposed fix.

          Show
          Kristian Waagan added a comment - Hi Wendy, Thanks for reporting the issue. The proposed fix sounds good, but before we implement it we may want to verify that we indeed need this data to be shared. As far as I can see, the id is used only for tracing. Having a local counter together with the id for the real connection would uniquely identify a pooled connection (would have to save this, since the real connection can be nullified), but we would then loose creation ordering information. In any case I think accessing the shared state in this case won't cause any performance issues (once per pooled connection creation, minimal critical section size). Opinions anyone? If I don't hear anything, I'll commit Wendy's proposed fix.
          Hide
          Knut Anders Hatlen added a comment -

          I agree that Wendy's proposed fix sounds fine.

          Since the field is only used to identify the instance uniquely in toString(), and toString() already prints the value of Object.hashCode(), perhaps we could just remove this code? Object.hashCode() isn't guaranteed to return a unique value, but it's probably good enough for tracing purposes.

          Show
          Knut Anders Hatlen added a comment - I agree that Wendy's proposed fix sounds fine. Since the field is only used to identify the instance uniquely in toString(), and toString() already prints the value of Object.hashCode(), perhaps we could just remove this code? Object.hashCode() isn't guaranteed to return a unique value, but it's probably good enough for tracing purposes.
          Hide
          Kristian Waagan added a comment -

          Attached patch 1a, which removes the code using incorrect synchronization.

          Show
          Kristian Waagan added a comment - Attached patch 1a, which removes the code using incorrect synchronization.
          Hide
          Kristian Waagan added a comment -

          Committed patch 1a to trunk with revision 980089.
          Regression tests passed (12836 tests executed).

          Show
          Kristian Waagan added a comment - Committed patch 1a to trunk with revision 980089. Regression tests passed (12836 tests executed).
          Hide
          Kathey Marsden added a comment -

          Reopen for backport

          Show
          Kathey Marsden added a comment - Reopen for backport
          Hide
          Kathey Marsden added a comment -

          Assign to myself temporarily for backport.

          Show
          Kathey Marsden added a comment - Assign to myself temporarily for backport.
          Hide
          Knut Anders Hatlen added a comment -

          [bulk update] Close all resolved issues that haven't been updated for more than one year.

          Show
          Knut Anders Hatlen added a comment - [bulk update] Close all resolved issues that haven't been updated for more than one year.

            People

            • Assignee:
              Kristian Waagan
              Reporter:
              Wendy Feng
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Time Tracking

                Estimated:
                Original Estimate - 10m
                10m
                Remaining:
                Remaining Estimate - 10m
                10m
                Logged:
                Time Spent - Not Specified
                Not Specified

                  Development