Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 1.0, 1.1, 1.2
    • Fix Version/s: 1.3
    • Component/s: CLI-1.x
    • Labels:
      None

      Description

      Class OptionBuilder is not thread-safe.
      It uses one OptionBuilder instance without synchronizing access to it.
      If more than one thread uses OptionBuilder, that can result in invalid Options (suppose a scheduler component).

      Don't know if this should be fixed. At least the Javadoc should mention the fact that the class is not thread-safe.
      As a workaround, use constructor of class Option.

        Activity

        Hide
        sebb@apache.org Sebb added a comment - - edited

        In fact the OptionBuilder code is at best serially re-usable.

        Even then, the second thread would need to ensure that reset() was called before starting to use the class, because the lack of synch. means that the static variables updated by one thread may not be published to the second thread - i.e. the second thread might see stale values even though the first thread called reset() as part of create(). And of course the threads would have to ensure that they took it in turns to use the class.

        Note that Option is not thread-safe either (settters/getters not synch.), however if the Option instance is confined to a single thread it would be OK.

        Many of the other classes are not thread-safe either, as they have mutable variables that aren't volatile or synch.
        But if confined to a thread, they would be OK.

        Unless the documentation says otherwise, it's not safe to assume that a lbrary is thread-safe.

        But I agree that the OptionBuilder is a particular problem.

        It could be fixed by using ThreadLocal for all the static variables. But is it worth the effort?

        Show
        sebb@apache.org Sebb added a comment - - edited In fact the OptionBuilder code is at best serially re-usable. Even then, the second thread would need to ensure that reset() was called before starting to use the class, because the lack of synch. means that the static variables updated by one thread may not be published to the second thread - i.e. the second thread might see stale values even though the first thread called reset() as part of create(). And of course the threads would have to ensure that they took it in turns to use the class. Note that Option is not thread-safe either (settters/getters not synch.), however if the Option instance is confined to a single thread it would be OK. Many of the other classes are not thread-safe either, as they have mutable variables that aren't volatile or synch. But if confined to a thread, they would be OK. Unless the documentation says otherwise, it's not safe to assume that a lbrary is thread-safe. But I agree that the OptionBuilder is a particular problem. It could be fixed by using ThreadLocal for all the static variables. But is it worth the effort?
        Hide
        ebourg Emmanuel Bourg added a comment -

        Just curious, what is the use case that would require making CLI thread safe? A typical application uses CLI on its main thread to parse the command line parameters, that's far from a multi thread scenario.

        Show
        ebourg Emmanuel Bourg added a comment - Just curious, what is the use case that would require making CLI thread safe? A typical application uses CLI on its main thread to parse the command line parameters, that's far from a multi thread scenario.
        Hide
        sebb@apache.org Sebb added a comment -

        Another way to fix the class would be to turn all the static variables into instance variables, and add parallel instance methods. The static methods would then use a static singleton instance. That would allow use from multiple threads without the overhead of ThreadLocal.

        The library would still not be thread-safe, but at least it could be used by multiple threads in parallel so long as each thread had its own instances.

        Show
        sebb@apache.org Sebb added a comment - Another way to fix the class would be to turn all the static variables into instance variables, and add parallel instance methods. The static methods would then use a static singleton instance. That would allow use from multiple threads without the overhead of ThreadLocal. The library would still not be thread-safe, but at least it could be used by multiple threads in parallel so long as each thread had its own instances.
        Hide
        ebourg Emmanuel Bourg added a comment -

        I updated the Javadoc as suggested by Thomas.

        Ideally I'd like to rewrite OptionBuilder with non static methods, beside the thread safety issue this cause my IDE to show a lot of ugly warnings, that's not a pleasant experience. Due to the incompatible nature of this modification it will have to wait for a major release. We might also deprecate the class and rewrite it under a new name, for example OptionsBuilder.

        Show
        ebourg Emmanuel Bourg added a comment - I updated the Javadoc as suggested by Thomas. Ideally I'd like to rewrite OptionBuilder with non static methods, beside the thread safety issue this cause my IDE to show a lot of ugly warnings, that's not a pleasant experience. Due to the incompatible nature of this modification it will have to wait for a major release. We might also deprecate the class and rewrite it under a new name, for example OptionsBuilder.
        Hide
        karel1980 Karel Vervaeke added a comment -

        I would mark this 'wont fix', since OptionBuilder still is not, by any definition threadsafe.
        CLI-224 seems to be a duplicate. Perhaps you could keep this one closed (won't fix in 1.x line) and keep the other one open for 2.0.

        Two hypothetical use cases for making CLI thread safe:

        • A container where multiple users enter commands simultaneously, and where the parsing of the command happens in the container).
        • A program designed to execute multiple commands in parallel: The commands are read from configuration files, and for each line a parallel process is started to parse and execute the command.
        Show
        karel1980 Karel Vervaeke added a comment - I would mark this 'wont fix', since OptionBuilder still is not, by any definition threadsafe. CLI-224 seems to be a duplicate. Perhaps you could keep this one closed (won't fix in 1.x line) and keep the other one open for 2.0. Two hypothetical use cases for making CLI thread safe: A container where multiple users enter commands simultaneously, and where the parsing of the command happens in the container). A program designed to execute multiple commands in parallel: The commands are read from configuration files, and for each line a parallel process is started to parse and execute the command.

          People

          • Assignee:
            Unassigned
            Reporter:
            therre Thomas Herre
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development