Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions org.knime.core/src/eclipse/org/knime/core/node/KNIMEConstants.java
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,22 @@ public final class KNIMEConstants {
*/
public static final String ENV_MAX_THREAD_COUNT = "KNIME_CORE_MAX_THREADS";

/**
* Environment variable controlling where the logging failsafe output is written.
*
* Supported values are {@code stderr} and {@code stdout}. The default is {@code stderr}, and any other value
* falls back to {@code stderr}.
*/
public static final String ENV_LOGGING_FAILSAFE_TARGET = "KNIME_CORE_LOGGING_FAILSAFE_TARGET";

/**
* Environment variable controlling the minimum level written by the logging failsafe output.
*
* Supported values are log4j levels such as {@code DEBUG}, {@code INFO}, {@code WARN}, {@code ERROR}, and
* {@code FATAL}. The default is {@code DEBUG}, and any other value falls back to {@code DEBUG}.
*/
public static final String ENV_LOGGING_FAILSAFE_MIN_LEVEL = "KNIME_CORE_LOGGING_FAILSAFE_MIN_LEVEL";

/**
* Java property name to specify the default temp directory for KNIME temp files (such as data files). This can be
* changed in the preference pages and is by default the same as the java.io.tmpdir
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,11 @@
import java.io.IOException;
import java.io.InputStream;
import java.io.InputStreamReader;
import java.io.PrintStream;
import java.io.Writer;
import java.nio.file.Files;
import java.nio.file.Path;
import java.time.Instant;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
Expand All @@ -68,6 +70,7 @@
import java.util.Objects;
import java.util.Optional;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.function.BiFunction;
import java.util.function.Supplier;

Expand All @@ -89,6 +92,7 @@
import org.knime.core.node.NodeLogger;
import org.knime.core.node.NodeLogger.LEVEL;
import org.knime.core.node.NodeLogger.NodeContextInformation;
import org.knime.core.node.NodeLoggerPatternLayout;
import org.knime.core.node.logging.LogBuffer.BufferedLogMessage;
import org.knime.core.node.util.CheckUtils;
import org.knime.core.node.workflow.WorkflowManager;
Expand Down Expand Up @@ -185,6 +189,18 @@ public void run() {

private static volatile boolean isInitialized;

private static final PrintStream FAILSAFE_LOGGING_TARGET = initFailsafeLoggingTarget();

private static final Level FAILSAFE_LOGGING_MIN_LEVEL = initFailsafeLoggingMinLevel();

private static final Layout FAILSAFE_LOGGING_LAYOUT = initFailsafeLoggingLayout();

private static final AtomicBoolean isShutdownHookInstalled = new AtomicBoolean();

private static volatile boolean isFailsafeShutdownLoggingActive;

private static boolean isFailsafeBufferDumped;

static boolean isInstanceLocationSet() {
final var loc = Platform.getInstanceLocation();
return loc != null && loc.isSet();
Expand Down Expand Up @@ -281,6 +297,108 @@ static synchronized void initializeLogging(final boolean logBufferedLogMessages)
}
}

private static void installFailsafeLoggingHandlers() {
if (isShutdownHookInstalled.compareAndSet(false, true)) {
try {
Runtime.getRuntime().addShutdownHook(new Thread(
() -> drainBufferedMessagesTo(FAILSAFE_LOGGING_TARGET,
"JVM is shutting down before KNIME logging could finish initialization"),
"KNIME startup log dump"));
} catch (IllegalStateException | SecurityException ex) {
isShutdownHookInstalled.set(false);
FAILSAFE_LOGGING_TARGET.println("Unable to register KNIME startup log shutdown hook:");
ex.printStackTrace(FAILSAFE_LOGGING_TARGET);
}
}
}

/**
* Drains buffered startup log messages to the given stream.
*
* @param out the target stream
* @param reason a human-readable reason included in the dump header
*/
private static void drainBufferedMessagesTo(final PrintStream out, final String reason) {
drainBufferedMessagesTo(out, reason, FAILSAFE_LOGGING_MIN_LEVEL);
}

private static void drainBufferedMessagesTo(final PrintStream out, final String reason, final Level minLevel) {
Objects.requireNonNull(out);
final LogBuffer.BufferContents contents;
synchronized (BUFFER) {
if (isFailsafeBufferDumped) {
return;
}
isFailsafeShutdownLoggingActive = true;
isFailsafeBufferDumped = true;
contents = BUFFER.drain();
}
if (contents.isEmpty()) {
return;
}

final var matchingMessages = Arrays.stream(contents.messages())
.filter(message -> message.level().isGreaterOrEqual(minLevel))
.toList();
final var shouldEmitEvictionNotice = contents.evictedEntries() > 0
&& contents.evictionMessageLevel().isGreaterOrEqual(minLevel);
if (matchingMessages.isEmpty() && !shouldEmitEvictionNotice) {
return;
}

final var total = contents.messages().length + contents.evictedEntries();
final var countMessages = total > 1 ? "%d messages were".formatted(total) : "1 message was";
out.printf("KNIME startup failsafe logging dump: %s. %s buffered. Minimum level: %s.%n",
reason, countMessages, minLevel);
if (shouldEmitEvictionNotice) {
emitToFailsafeLoggingTarget(KNIMELogger.class.getName(), contents.evictionMessageLevel(),
"[*** Log incomplete: log buffer did wrap around -- %d messages were evicted from buffer in total ***]"
.formatted(contents.evictedEntries()),
null, Instant.now());
}
for (final var message : matchingMessages) {
emitToFailsafeLoggingTarget(message.name(), message.level(), message.message(), message.cause(),
message.instant());
}
Comment on lines +351 to +362
Copy link

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

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

drainBufferedMessagesTo(PrintStream out, ...) writes the header/footer to out, but the actual log events are emitted via emitToFailsafeLoggingTarget(...), which always prints to the global FAILSAFE_LOGGING_TARGET. This can split the dump across two streams if out is ever different, and makes the out parameter misleading. Consider either emitting everything to out (pass it into emit...) or removing/renaming the out parameter to reflect the fixed target.

Copilot uses AI. Check for mistakes.
out.println("End of KNIME startup failsafe logging dump.");
out.flush();
}

private static void emitToFailsafeLoggingTarget(final String name, final Level level, final Object message,
final Throwable cause) {
if (!level.isGreaterOrEqual(FAILSAFE_LOGGING_MIN_LEVEL)) {
return;
}

emitToFailsafeLoggingTarget(name, level, message, cause, Instant.now());
}

private static void emitToFailsafeLoggingTarget(final String name, final Level level, final Object message,
final Throwable cause, final Instant instant) {
final var event = new LoggingEvent(name, Logger.getLogger(name), instant.toEpochMilli(), level, message, cause);
FAILSAFE_LOGGING_TARGET.print(FAILSAFE_LOGGING_LAYOUT.format(event));
if (cause != null && FAILSAFE_LOGGING_LAYOUT.ignoresThrowable()) {
cause.printStackTrace(FAILSAFE_LOGGING_TARGET);
}
FAILSAFE_LOGGING_TARGET.flush();
}

private static PrintStream initFailsafeLoggingTarget() {
final var configuredTarget = System.getenv(KNIMEConstants.ENV_LOGGING_FAILSAFE_TARGET);
return "stdout".equalsIgnoreCase(configuredTarget) ? System.out : System.err;
}

private static Level initFailsafeLoggingMinLevel() {
return Level.toLevel(System.getenv(KNIMEConstants.ENV_LOGGING_FAILSAFE_MIN_LEVEL), Level.DEBUG);
}

private static Layout initFailsafeLoggingLayout() {
final var layout = new NodeLoggerPatternLayout();
layout.setConversionPattern("%d{ISO8601} : %-5p : %t : %c{1} : %m%n");
layout.activateOptions();
return layout;
}

private void initializeInternalLogger() {
if (m_logger == null) {
m_logger = new DelegatingLogger(m_name);
Expand Down Expand Up @@ -984,12 +1102,22 @@ private void log(final BufferedLogMessage bufferedMessage) {
*/
public void log(final Level level, final Supplier<Object> messageSupplier, final boolean omitContext,
final Throwable cause) {
Objects.requireNonNull(level, "Must not log a null level");
// we double-check to avoid expensive locking
if (!isInitialized()) {
if (isFailsafeShutdownLoggingActive) {
emitToFailsafeLoggingTarget(m_name, level, messageSupplier.get(), cause);
return;
Comment on lines +1108 to +1110
Copy link

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

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

In failsafe-shutdown mode, this evaluates messageSupplier.get() unconditionally before the level filter is applied inside emitToFailsafeLoggingTarget(...). That can introduce unexpected side effects and unnecessary work for messages below FAILSAFE_LOGGING_MIN_LEVEL. Consider checking level.isGreaterOrEqual(FAILSAFE_LOGGING_MIN_LEVEL) (or adding a supplier-based overload) before invoking the supplier.

Copilot uses AI. Check for mistakes.
}
installFailsafeLoggingHandlers();
synchronized(BUFFER) {
// we need to check again now that we have the lock to see if someone else has initialized it in the
// meantime
if (!isInitialized()) {
if (isFailsafeShutdownLoggingActive) {
emitToFailsafeLoggingTarget(m_name, level, messageSupplier.get(), cause);
return;
}
Comment on lines +1117 to +1120
Copy link

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

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

Same issue as above: when isFailsafeShutdownLoggingActive becomes true between the outer and inner initialization checks, messageSupplier.get() is still evaluated even if the message would be filtered out by FAILSAFE_LOGGING_MIN_LEVEL. Add a level check before invoking the supplier (or route through a supplier-aware emit method) to preserve lazy-evaluation semantics.

Copilot uses AI. Check for mistakes.
BUFFER.log(level, m_name, messageSupplier.get(), cause);
return;
}
Expand All @@ -1008,10 +1136,20 @@ public void log(final Level level, final Supplier<Object> messageSupplier, final
*/
public void log(final Level level, final Object message, final boolean omitContext,
final Throwable cause) {
Objects.requireNonNull(level, "Must not log a null level");
// we double-check to avoid expensive locking
if (!isInitialized()) {
if (isFailsafeShutdownLoggingActive) {
emitToFailsafeLoggingTarget(m_name, level, message, cause);
return;
}
installFailsafeLoggingHandlers();
synchronized(BUFFER) {
if (!isInitialized()) {
if (isFailsafeShutdownLoggingActive) {
emitToFailsafeLoggingTarget(m_name, level, message, cause);
return;
}
BUFFER.log(level, m_name, message, cause);
return;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,19 @@
*/
final class LogBuffer {

/**
* Drained buffer contents and any overflow metadata.
*
* @param messages buffered messages in insertion order
* @param evictedEntries total number of messages evicted from the buffer
* @param evictionMessageLevel log level to use for an overflow notice
*/
record BufferContents(BufferedLogMessage[] messages, long evictedEntries, Level evictionMessageLevel) {
boolean isEmpty() {
return messages.length == 0;
}
}

private final CircularFiFoBuffer m_logBuffer;

LogBuffer(final int bufferSize) {
Expand Down Expand Up @@ -187,26 +200,48 @@ private int size() {
* @param consumer consumer for buffered log messages
*/
void drainTo(final Consumer<BufferedLogMessage> consumer) {
if (!m_logBuffer.isEmpty()) {
final var contents = drain();
if (!contents.isEmpty()) {
Comment on lines 202 to +204
Copy link

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

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

drainTo() calls drain(), but drain() explicitly documents that callers must synchronize on the LogBuffer instance. drainTo() itself neither synchronizes nor documents the same requirement, so it violates drain()'s contract and is unsafe if invoked without external locking. Consider making drain() (and/or drainTo()) synchronized, or synchronizing internally on this, and update the Javadoc accordingly.

Copilot uses AI. Check for mistakes.
// we expect no NodeContext to be available at this point anyway
final var omitCtx = true;
final var logger = KNIMELogger.getLogger(KNIMELogger.class);
final var current = m_logBuffer.size();
final var evicted = m_logBuffer.getNumberOfEvictedEntries();
final var total = current + evicted;
final var total = contents.messages().length + contents.evictedEntries();
final var countMessages = total > 1 ? "%d messages were".formatted(total) : "1 message was";
logger.log(Level.DEBUG, () -> "%s logged before logging was initialized; see below..."
.formatted(countMessages), omitCtx, null);
if (evicted > 0) {
logger.log(m_logBuffer.m_levelForEvictionMessage,
if (contents.evictedEntries() > 0) {
logger.log(contents.evictionMessageLevel(),
() -> "[*** Log incomplete: log buffer did wrap around -- "
+ "%d messages were evicted from buffer in total ***]".formatted(evicted), omitCtx, null);
+ "%d messages were evicted from buffer in total ***]"
.formatted(contents.evictedEntries()),
omitCtx, null);
}
for (final var message : contents.messages()) {
consumer.accept(message);
}
m_logBuffer.drainingIterator().forEachRemaining(consumer);
logger.log(Level.DEBUG, "End of buffered log messages", omitCtx, null);
}
}

/**
* Drains and returns the current buffer contents.
*
* <p>
* <b>Note:</b> this method is <i>not thread-safe</i>. Callers must synchronize on the parent
* {@link LogBuffer} instance before invoking it.
*
* @return drained buffer contents in insertion order
*/
BufferContents drain() {
final var current = m_logBuffer.size();
final var messages = new BufferedLogMessage[current];
final var iter = m_logBuffer.drainingIterator();
for (var i = 0; i < current; i++) {
messages[i] = iter.next();
}
return new BufferContents(messages, m_logBuffer.getNumberOfEvictedEntries(), m_logBuffer.m_levelForEvictionMessage);
}

/**
* Buffer the given message at the given level under the given logger name.
*
Expand Down
Loading