Uploaded image for project: 'Spring Framework'
  1. Spring Framework
  2. SPR-13617

AbstractJdbcCall's compiled variable should be declared as volatile

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Complete
    • Affects Version/s: 3.2.15, 4.1.8, 4.2.2
    • Fix Version/s: 3.2.16, 4.1.9, 4.2.3
    • Component/s: Data:JDBC
    • Labels:
      None
    • Last commented by a User:
      false

      Description

      AbstractJdbcCall contains a Double-Checked Locking code over the compiled field.
      It should be declared volatile to avoid ordering issues that can result in race-condition scenarios.

        Activity

        Hide
        juergen.hoeller Juergen Hoeller added a comment -

        Fair enough, we'll declare it as volatile.

        However, please note that the effect is rather minimal: Since compilation is a one-time change in such an instance (i.e. an operation is never going to get un-compiled or re-compiled), other threads may miss an update to the compiled variable when it switches from false to true (which is an atomic operation with fully constructed state to be seen at any point). With checkCompiled(), this means it will unnecessarily invoke compile() - which checks the compiled flag again anyway, but within a synchronized block. So effectively, we optimize access via volatile to consistently avoid full synchronization...

        Juergen

        Show
        juergen.hoeller Juergen Hoeller added a comment - Fair enough, we'll declare it as volatile . However, please note that the effect is rather minimal: Since compilation is a one-time change in such an instance (i.e. an operation is never going to get un-compiled or re-compiled), other threads may miss an update to the compiled  variable when it switches from false to true (which is an atomic operation with fully constructed state to be seen at any point). With checkCompiled() , this means it will unnecessarily invoke compile() - which checks the compiled flag again anyway, but within a synchronized block. So effectively, we optimize access via volatile to consistently avoid full synchronization... Juergen
        Hide
        shimib77 Shimi Bandiel added a comment -

        I think that the really dangerous scenario can happen is the CPU will re-order the write to the compiled flag to a place before the 'compilation-logic' happens.
        In that case another thread may use an non-compiled version of the object.

        Show
        shimib77 Shimi Bandiel added a comment - I think that the really dangerous scenario can happen is the CPU will re-order the write to the compiled flag to a place before the 'compilation-logic' happens. In that case another thread may use an non-compiled version of the object.
        Hide
        juergen.hoeller Juergen Hoeller added a comment -

        Good point; I haven't seen this happening yet but it might theoretically occur.

        The common version of the problem is delayed exposure of the new value to other threads since the CPU cache contains the stale old version still... even that is worth addressing since we don't want to enter full synchronization without a hard reason here.

        Juergen

        Show
        juergen.hoeller Juergen Hoeller added a comment - Good point; I haven't seen this happening yet but it might theoretically occur. The common version of the problem is delayed exposure of the new value to other threads since the CPU cache contains the stale old version still... even that is worth addressing since we don't want to enter full synchronization without a hard reason here. Juergen
        Hide
        juergen.hoeller Juergen Hoeller added a comment -

        I've addressed the same problem in AbstractJdbcInsert and RdbmsOperation as well.

        Juergen

        Show
        juergen.hoeller Juergen Hoeller added a comment - I've addressed the same problem in AbstractJdbcInsert and RdbmsOperation as well. Juergen

          People

          • Assignee:
            juergen.hoeller Juergen Hoeller
            Reporter:
            shimib77 Shimi Bandiel
            Last updater:
            Stéphane Nicoll
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:
              Days since last comment:
              2 years, 17 weeks, 3 days ago