Jetty
  1. Jetty
  2. JETTY-1187

Non-atomic self-increment operation on volatile field _set in class SelectorManager

    Details

    • Type: Improvement Improvement
    • Status: Resolved Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 7.1.0
    • Fix Version/s: None
    • Component/s: NIO
    • Labels:
      None
    • Number of attachments :
      0

      Description

      In class SelectorManager, field "_set" is declared as volatile int, in method register(xxx), _set++ is seen, which is a three-step (read->modify->write) non-atomic operation. Volatile keyword only guarantees variable visibility, but not atomicity, so _set++ can result to race condition and incorrect result. It's suggested to use synchronized block to protect _set++ or AtomicInteger class from java.util.concurrent package to do atomic increment operation.

        Activity

        Hide
        Greg Wilkins added a comment -

        The usage of the _set++ operation is to scatter the newly accepted keys over the available SelectSets. It is not vitally important that the increment happens every operation and a bit of randomness would not hurt either.

        However, this behaviour should either be commented on in the code, or we should use an AtomicInteger - as I doubt the cost of that is at all different from a ++ on a volatile... it might even be better.

        Probably will comment existing behaviour in jetty-6 and use atomic integer in jetty-7

        Show
        Greg Wilkins added a comment - The usage of the _set++ operation is to scatter the newly accepted keys over the available SelectSets. It is not vitally important that the increment happens every operation and a bit of randomness would not hurt either. However, this behaviour should either be commented on in the code, or we should use an AtomicInteger - as I doubt the cost of that is at all different from a ++ on a volatile... it might even be better. Probably will comment existing behaviour in jetty-6 and use atomic integer in jetty-7
        Hide
        Greg Wilkins added a comment -

        added a comment in jetty-7 saying that it is known not atomic

        Show
        Greg Wilkins added a comment - added a comment in jetty-7 saying that it is known not atomic

          People

          • Assignee:
            Greg Wilkins
            Reporter:
            Daniel Luo
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: