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 #25220 The number of Logging pump threads grows infinitely #25221

Merged
merged 1 commit into from
Nov 21, 2024

Conversation

kaido207
Copy link
Contributor

@kaido207 kaido207 commented Nov 21, 2024

Fix to destroy old threads through LoggingOutputStream.close() when updating contents of logging.properties.

First, fix LoggingOutputStream.close() to be called when logging.properties is updated.
When logging.properties is updated, GlassFishLogHandler.reconfigure() is called to recreate the LoggingOutputStream based on the updated contents of logging.properties.

public void reconfigure(final GlassFishLogHandlerConfiguration newConfiguration) {
trace(GlassFishLogHandler.class, () -> "reconfigure(configuration=" + newConfiguration + ")");
lock.lock();
try {
// stop using output, but allow collecting records. Logging system can continue to work.
this.status = GlassFishLogHandlerStatus.ACCEPTING;
this.logRecordBuffer.reconfigure(newConfiguration.getBufferCapacity(), newConfiguration.getBufferTimeout());
if (this.rotationTimerTask != null) {
// to avoid another task from last configuration runs it's action.
this.rotationTimerTask.cancel();
this.rotationTimerTask = null;
}
// stop pump. If reconfiguration would fail, it is better to leave it down.
// records from the buffer will be processed if the last configuration was valid.
stopPump();
this.configuration = newConfiguration;
try {
this.status = startLoggingIfPossible();
} catch (final Exception e) {
this.status = GlassFishLogHandlerStatus.OFF;
throw e;
}
} finally {
lock.unlock();
}
}

However, there is no process to stop the existing LoggingOutputStream's Pump Thread during this recreation.

Second, fix processing so that the Pump Thread is destroyed when LoggingOutputStream.close() is called.
When LoggingOutputStream.close() is called, Pump.shutdown() is called, but Pump.shutdown() cannot stop the Pump thread.

void shutdown() {
this.interrupt();
// we interrupted waiting or working thread, now we have to process remaining records.
logAllPendingRecords();
}

If Pump.interrupt() is called, the implementation absorbs the InterruptException inside the LogRecordBuffer class.
public GlassFishLogRecord pollOrWait() {
GlassFishLogRecord logRecord = null;
try {
logRecord = pendingRecords.take();
availableCapacity.release();
} catch (InterruptedException e) {
// do nothing
}
return logRecord;
}

Therefore, the while loop of the Pump thread does not stop when Pump.shutdown() is called.
So, the fix is to add a flag and modify Pump.shutdown() to stop Thread.

Signed-off-by: kaido207 <kaido.hiroki@fujitsu.com>
Copy link
Contributor

@dmatej dmatej left a comment

Choose a reason for hiding this comment

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

Hmm, seems there conflicted two ideas in the original solution:

  • That STDOUT+STDERR never close
  • That GlassFishLogHandler can close and can reconfigure (and disable/enable processing of STD output)

Then your fix is correct.

@dmatej dmatej added this to the 7.0.20 milestone Nov 21, 2024
@dmatej dmatej added the bug Something isn't working label Nov 21, 2024
@dmatej dmatej merged commit 6177d58 into eclipse-ee4j:master Nov 21, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The number of Logging pump threads grows infinitely
3 participants