Details

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

      Description

      The LogKitLogger.logger field is not final or volatile so changes are not guaranteed to be published.
      This includes calls to getLogger(), so two different threads using the same instance can theoretically both create the logger.

        Activity

        Hide
        Sebb added a comment -

        Other problem fields include:

        AvalonLogger.defaultLogger

        SimpleLog - most fields

        Log4JLogger.logger

        Show
        Sebb added a comment - Other problem fields include: AvalonLogger.defaultLogger SimpleLog - most fields Log4JLogger.logger
        Hide
        Thomas Neidhart added a comment -

        Can this really happen, as the logger field is set in the constructor?
        A second thread would only be able to use the LogKitLogger object after it has been created.

        Show
        Thomas Neidhart added a comment - Can this really happen, as the logger field is set in the constructor? A second thread would only be able to use the LogKitLogger object after it has been created.
        Hide
        Sebb added a comment -

        If thread A creates the class and passes it to thread B (which already exists) there is no guarantee that thread B will see the same value as written by thread A (this is a consequence of the Java memory model, which allows for lots of hardware optimisations).

        Furthermore, the field is protected, so it can be changed at any later time. Unless the writer and reader threads synchronise on the same lock (or the field is made volatile), there is no guarantee that thread B will see what thread A wrote.

        Show
        Sebb added a comment - If thread A creates the class and passes it to thread B (which already exists) there is no guarantee that thread B will see the same value as written by thread A (this is a consequence of the Java memory model, which allows for lots of hardware optimisations). Furthermore, the field is protected, so it can be changed at any later time. Unless the writer and reader threads synchronise on the same lock (or the field is made volatile), there is no guarantee that thread B will see what thread A wrote.
        Hide
        Thomas Neidhart added a comment -

        I do not think the respective logger will be created multiple times, but retrieved from the underlying logging system.
        In case the underlying system is flawed (e.g. not returning the same instance everytime it is called with the same name), this may create problems in the commons-logging wrapper, but otherwise the synchronisation may be a bottleneck.

        This only relates to the logger fields, SimpleLog may be a different case.

        Show
        Thomas Neidhart added a comment - I do not think the respective logger will be created multiple times, but retrieved from the underlying logging system. In case the underlying system is flawed (e.g. not returning the same instance everytime it is called with the same name), this may create problems in the commons-logging wrapper, but otherwise the synchronisation may be a bottleneck. This only relates to the logger fields, SimpleLog may be a different case.
        Hide
        Sebb added a comment -

        I agree it's unlikely to occur, however that does not mean it cannot occur.
        And if it does cause problems, debugging is likely to be extremely difficult.

        It may well be that all logger implementations return a singleton.
        But writing to the protected logger field can still cause issues.

        At the very least the issue should be clearly documented.

        Making the variable volatile would solve the publication issue.
        Ideally the variables should be private and final, but that would break compatibility.

        If logging is ever rewritten, it should use immutable classes as far as possible, and certainly no mutable protected or public fields.

        Show
        Sebb added a comment - I agree it's unlikely to occur, however that does not mean it cannot occur. And if it does cause problems, debugging is likely to be extremely difficult. It may well be that all logger implementations return a singleton. But writing to the protected logger field can still cause issues. At the very least the issue should be clearly documented. Making the variable volatile would solve the publication issue. Ideally the variables should be private and final, but that would break compatibility. If logging is ever rewritten, it should use immutable classes as far as possible, and certainly no mutable protected or public fields.
        Hide
        Thomas Neidhart added a comment -

        Yes I agree, also it is not clear why it is protected in the first place.
        Some of the wrappers have it private (log4j, avalon), while others have it private.
        Imho, there should be no reason to make it protected, and this should be fixed in future releases.

        Show
        Thomas Neidhart added a comment - Yes I agree, also it is not clear why it is protected in the first place. Some of the wrappers have it private (log4j, avalon), while others have it private. Imho, there should be no reason to make it protected, and this should be fixed in future releases.
        Hide
        Thomas Neidhart added a comment -

        Looking at the Log4JLogger, wouldn't it be better to always set the underlying logger in the constructor, and change the getLogger method in a way to just return the logger (similar to the AvalonLogger)?

        The null check looks unnecessary, as it should always be set in the constructor (its missing in the standard ctor, but could be added there too).

        The only thing that comes to my mind is that at creation time, the underlying log system may return null, but this would be odd anyway.

        Show
        Thomas Neidhart added a comment - Looking at the Log4JLogger, wouldn't it be better to always set the underlying logger in the constructor, and change the getLogger method in a way to just return the logger (similar to the AvalonLogger)? The null check looks unnecessary, as it should always be set in the constructor (its missing in the standard ctor, but could be added there too). The only thing that comes to my mind is that at creation time, the underlying log system may return null, but this would be odd anyway.
        Hide
        Thomas Neidhart added a comment -

        Applied first bunch of changes in r1435115:

        • SimpleLog:
          • made fields logName and shortLogName volatile -> should become private final in next major version
          • made modifiable field currentLogLevel volatile
          • made protected static fields showLogName, showShortName, showDateTime and dateTimeFormat volatile -> should become private in next major version with static getters?
        • AvalonLogger:
          • made static field defaultLogger volatile: there is already a static setter, so this should be fine

        Open:

        • Log4JLogger: logger field
        • LogKitLogger: logger field

        I would prefer to change the logger to a final field (Log4JLogger), always set the logger in the ctors and change the getLogger to just return the field. For LogKitLogger we can not set it final as it is a protected field, so adding volatile for now, and postpone for the next major version to change it to private.

        Show
        Thomas Neidhart added a comment - Applied first bunch of changes in r1435115: SimpleLog: made fields logName and shortLogName volatile -> should become private final in next major version made modifiable field currentLogLevel volatile made protected static fields showLogName, showShortName, showDateTime and dateTimeFormat volatile -> should become private in next major version with static getters? AvalonLogger: made static field defaultLogger volatile: there is already a static setter, so this should be fine Open: Log4JLogger: logger field LogKitLogger: logger field I would prefer to change the logger to a final field (Log4JLogger), always set the logger in the ctors and change the getLogger to just return the field. For LogKitLogger we can not set it final as it is a protected field, so adding volatile for now, and postpone for the next major version to change it to private.
        Hide
        Thomas Neidhart added a comment -

        I missed that the loggers are serializable, thus the logic in the getLogger() makes sense (no need to re-initialize the transient logger fields in a readObject method, as it will be checked each access to getLogger).

        Show
        Thomas Neidhart added a comment - I missed that the loggers are serializable, thus the logic in the getLogger() makes sense (no need to re-initialize the transient logger fields in a readObject method, as it will be checked each access to getLogger).
        Hide
        Thomas Neidhart added a comment -

        Applied the double-checked locking idiom from Josh Bloch (Item 71) for Log4JLogger and LogKitLogger (and changing the logger variable to volatile).

        This is not working reliably for pre-1.5 java but at least better than before.
        We could also make the getLogger() methods synchronized, but this may have a negative effect on performance in certain environments.

        Show
        Thomas Neidhart added a comment - Applied the double-checked locking idiom from Josh Bloch (Item 71) for Log4JLogger and LogKitLogger (and changing the logger variable to volatile). This is not working reliably for pre-1.5 java but at least better than before. We could also make the getLogger() methods synchronized, but this may have a negative effect on performance in certain environments.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development