Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix thread safety issues within BasicStatusManager #851

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mches
Copy link

@mches mches commented Aug 24, 2024

Fixes thread safety issues within BasicStatusManager, with respect to count and level. As it stands, updates to count and level could be temporarily or permanently invisible to other threads, or simply lost, due to lack of synchronization and non-atomic read-modify-write operations. Adopts the well-suited LongAdder for count and AtomicInteger for level. Without synchronization, level needs to be set using CAS to avoid a race.

@mches mches force-pushed the feature/fix-thread-safety branch 3 times, most recently from d641b43 to 99a3726 Compare September 9, 2024 06:43
Comment on lines +62 to +70
count.increment();
int newLevel = newStatus.getLevel();
int currentLevel;
do {
currentLevel = level.get();
if (newLevel <= currentLevel) {
break;
}
} while (!level.compareAndSet(currentLevel, newLevel));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about

synchronized (statusListLock) {
      count++;
       if (newStatus.getLevel() > level) {
           level = newStatus.getLevel();
       }
       if (statusList.size() < MAX_HEADER_COUNT) {
          statusList.add(newStatus);
      } else {
          tailBuffer.add(newStatus);
      }
  }

The code might be more simple.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we certainly could take advantage of the existing lock that's going to be taken out.

However, for correctness, the reads would also need to use the same lock:

    public int getLevel() {
        synchronized (statusListLock) {
            return level;
        }
    }

    public int getCount() {
        synchronized (statusListLock) {
            return count;
        }
    }

Otherwise the fields would need to be made volatile:

    volatile int count = 0;
    volatile int level = Status.INFO;

It also occurred to me that perhaps the level should be reset when the count is reset:

    public void clear() {
        synchronized (statusListLock) {
            count = 0;
            level = Level.INFO;
            statusList.clear();
            tailBuffer.clear();
        }
    }

@mches mches changed the title Fix thread safety Fix thread safety issues in BasicStatusManager Oct 14, 2024
@mches mches changed the title Fix thread safety issues in BasicStatusManager Fix thread safety issues within BasicStatusManager Oct 14, 2024
Signed-off-by: Mark Chesney <mches@users.noreply.github.com>
Comment on lines +62 to 72
count.increment();
int newLevel = newStatus.getLevel();
int currentLevel;
do {
currentLevel = level.get();
if (newLevel <= currentLevel) {
break;
}
} while (!level.compareAndSet(currentLevel, newLevel));

synchronized (statusListLock) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an alternate approach to CAS

Suggested change
count.increment();
int newLevel = newStatus.getLevel();
int currentLevel;
do {
currentLevel = level.get();
if (newLevel <= currentLevel) {
break;
}
} while (!level.compareAndSet(currentLevel, newLevel));
synchronized (statusListLock) {
synchronized (statusListLock) {
count.increment();
int newLevel = newStatus.getLevel();
if (newLevel > level.get()) {
level.set(newLevel);
}

@@ -90,18 +97,18 @@ private void fireStatusAddEvent(Status status) {

public void clear() {
synchronized (statusListLock) {
count = 0;
count.reset();
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should level be reset as well?

Suggested change
count.reset();
count.reset();
level.set(Status.INFO);

@@ -90,18 +97,18 @@ private void fireStatusAddEvent(Status status) {

public void clear() {
synchronized (statusListLock) {
count = 0;
count.reset();
statusList.clear();
tailBuffer.clear();
}
}

public int getLevel() {
Copy link
Author

@mches mches Oct 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should StatusManager#getLevel() be uncommented?

package ch.qos.logback.core.status;
…
public interface StatusManager {
…
    /**
     * Return the highest level of all the statii.
     * 
     * @return
     */
    // int getLevel();
…
}

Should @Override be added to all implemented methods? (Available since Java 6)

See also #845

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants