Commons Pool
  1. Commons Pool
  2. POOL-137

Inconsistent synchronization in GenericObjectPool; constant fields should be final

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.5
    • Labels:
      None

      Description

      _whenExhaustedAction is not always accessed using synchronization.

      The private static Latch class appears to be used across threads, but has no synchronization.

      The static class Config contains several fields which appear to be used as constants, however the fields are not final.

        Activity

        Hide
        Phil Steitz added a comment -

        Fixed in 1.5 release.

        Show
        Phil Steitz added a comment - Fixed in 1.5 release.
        Hide
        Mark Thomas added a comment -

        Re: _whenExhaustedAction : Already documented in-line

        Re: Latch: Syncs added.

        Show
        Mark Thomas added a comment - Re: _whenExhaustedAction : Already documented in-line Re: Latch: Syncs added.
        Hide
        Sebb added a comment -

        Re: _whenExhaustedAction : this should be documented for later maintainers please.

        Access to Latch fields need to be synch. regardless of whether the access is simultaneous or not - if one thread sets a field, another thread may not see the same value in the absence of synch.

        Re: Config - I thought the fields were used as constants, but on looking again they are used as defaults - so please ignore that comment.

        Show
        Sebb added a comment - Re: _whenExhaustedAction : this should be documented for later maintainers please. Access to Latch fields need to be synch. regardless of whether the access is simultaneous or not - if one thread sets a field, another thread may not see the same value in the absence of synch. Re: Config - I thought the fields were used as constants, but on looking again they are used as defaults - so please ignore that comment.
        Hide
        Mark Thomas added a comment -

        I have fixed the sync issues as best I can even though I am not a huge fan of my solution. I think the whole sync strategy needs to be re-thought for 2.0. I'll write up something for the dev list when we reach that point.

        Show
        Mark Thomas added a comment - I have fixed the sync issues as best I can even though I am not a huge fan of my solution. I think the whole sync strategy needs to be re-thought for 2.0. I'll write up something for the dev list when we reach that point.
        Hide
        Mark Thomas added a comment -

        I can't sync _whenExhaustedAction where it is used or I end up with a deadlock when running the tests. I think what I'll do is get a local copy of all the fields I need at the start of borrowObject() - in a sync block - so at least we are working with a consistent configuration for the rest of that method even if it might be out of date.

        For the use of Latch, I am happy (at the momentat least ) that the code is written in such a way that only one thread will ever be trying to access the latch at the same time. If you can see a code path where that isn't the case please add it here.

        The Config class looks OK to me. Which fields were you worried about?

        Show
        Mark Thomas added a comment - I can't sync _whenExhaustedAction where it is used or I end up with a deadlock when running the tests. I think what I'll do is get a local copy of all the fields I need at the start of borrowObject() - in a sync block - so at least we are working with a consistent configuration for the rest of that method even if it might be out of date. For the use of Latch, I am happy (at the momentat least ) that the code is written in such a way that only one thread will ever be trying to access the latch at the same time. If you can see a code path where that isn't the case please add it here. The Config class looks OK to me. Which fields were you worried about?

          People

          • Assignee:
            Unassigned
            Reporter:
            Sebb
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development