Skip to content

Conversation

vy
Copy link
Member

@vy vy commented Feb 10, 2024

The issue was initially caused by #2249. This fix also addresses the CI failures on Windows.

@vy vy added the bug Incorrect, unexpected, or unintended behavior of existing code label Feb 10, 2024
@vy vy added this to the 2.23.0 milestone Feb 10, 2024
@vy vy requested a review from ppkarwasz February 10, 2024 20:53
@vy vy self-assigned this Feb 10, 2024
@vy vy force-pushed the 2.x-StatusLogger-reset-fix branch from a226860 to 90cf8e7 Compare February 10, 2024 20:54
Copy link
Contributor

@ppkarwasz ppkarwasz left a comment

Choose a reason for hiding this comment

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

This looks good to me: it solves the immediate problem, although I would probably prefer for StatusConsoleListeners to be replaced instead of reconfigured.

The problem of resetting the StatusConsoleListener is however more general: since the constructors of several Configuration objects mess with the fallback StatusConsoleListener, logger contexts should call StatusLogger.reset() at shutdown:

final Configuration prev = configuration;
configuration = NULL_CONFIGURATION;
updateLoggers();
if (prev instanceof LifeCycle2) {
((LifeCycle2) prev).stop(timeout, timeUnit);
} else {
prev.stop();
}

If we fix the lines above, most calls to StatusLogger#reset in our tests should become useless.

I am not sure what to do during reconfiguration:

config.start();
this.configuration = config;
updateLoggers();
if (prev != null) {
prev.removeListener(this);
prev.stop();
}

Here the new configuration already change the fallback status listener settings, so we probably don't need to do anything.

@vy
Copy link
Member Author

vy commented Feb 12, 2024

This looks good to me: it solves the immediate problem, although I would probably prefer for StatusConsoleListeners to be replaced instead of reconfigured.

I explained why this is not possible in this conversation.

logger contexts should call StatusLogger.reset() at shutdown

StatusLogger is [statically] shared between all LoggerContexts. Hence, are you sure resetting the StatusLogger singleton when a LoggerContext shuts down is the right thing to do?

@vy
Copy link
Member Author

vy commented Feb 12, 2024

After a video call with @ppkarwasz, we decided to proceed with this PR as it is. A good step forward would be to remove StatusConfiguration in main.

@vy vy merged commit e87064f into 2.x Feb 12, 2024
@vy vy deleted the 2.x-StatusLogger-reset-fix branch February 12, 2024 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Incorrect, unexpected, or unintended behavior of existing code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants