fix: crash when using the logger outside of the a request context by dunglas · Pull Request #2087

fix: crash when using the logger outside of the a request context by dunglas · Pull Request #2087 · php/frankenphp

Comment on lines +664 to +674

		ctxHolder := phpThreads[threadIndex]
		if ctxHolder == nil {
		return globalLogger, globalCtx
		}

		fCtx := ctxHolder.frankenPHPContext()
		if fCtx == nil {
		return globalLogger, globalCtx
		}

		return fCtx.logger, ctxHolder.context()

Choose a reason for hiding this comment

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

Would it make sense to try to keep the request context when it exists?

ctxHolder := phpThreads[threadIndex]
if ctxHolder == nil {
	return globalLogger, globalCtx
}

fCtx := ctxHolder.frankenPHPContext()
if fCtx == nil || fCtx.logger == nil {
	return globalLogger, ctxHolder.context()
}

return fCtx.logger, ctxHolder.context()

Choose a reason for hiding this comment

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

It's already what is done, or am I missing something?

Choose a reason for hiding this comment

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

if fCtx == nil || fCtx.logger == nil {
	return globalLogger, ctxHolder.context()
}

Small difference here, you return globalCtx while there might be something interesting in ctxHolder.context(), as once there, ctxHolder is not nil, don't you think?

Choose a reason for hiding this comment

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

I don't understand how. contextHolder always contains a context and a frankenphpContext or is nil.

Choose a reason for hiding this comment

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

Ok, I see. Sorry, I am not too familiar with how this implemented behind the scenes, so I was just commenting about the conditionals, but I believe you are right, this may never happen in practice.

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a crash when using the logger outside of a request context by introducing a new getLogger helper function that safely retrieves the logger and context, falling back to global defaults when thread context is unavailable.

Key Changes:

Introduced getLogger function to safely handle logger retrieval with nil checks
Refactored go_log and go_log_attrs to use the new safe getLogger function

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +663 to +675

		func getLogger(threadIndex C.uintptr_t) (*slog.Logger, context.Context) {
		ctxHolder := phpThreads[threadIndex]
		if ctxHolder == nil {
		return globalLogger, globalCtx
		}

		fCtx := ctxHolder.frankenPHPContext()
		if fCtx == nil {
		return globalLogger, globalCtx
		}

		return fCtx.logger, ctxHolder.context()
		}

Choose a reason for hiding this comment

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

The getLogger function doesn't check if phpThreads slice is nil or if threadIndex is out of bounds before accessing phpThreads[threadIndex]. This could cause a panic if the logger is used after drainPHPThreads() sets phpThreads = nil or if an invalid threadIndex is passed.

Consider adding bounds checking:

Check if phpThreads is nil
Check if threadIndex is within the valid range (0 to len(phpThreads)-1)

Copilot uses AI. Check for mistakes.

Comment on lines +663 to +675

		func getLogger(threadIndex C.uintptr_t) (*slog.Logger, context.Context) {
		ctxHolder := phpThreads[threadIndex]
		if ctxHolder == nil {
		return globalLogger, globalCtx
		}

		fCtx := ctxHolder.frankenPHPContext()
		if fCtx == nil {
		return globalLogger, globalCtx
		}

		return fCtx.logger, ctxHolder.context()
		}

Choose a reason for hiding this comment

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

The new getLogger function that handles logging outside of a request context should have test coverage to ensure it correctly falls back to globalLogger and globalCtx when phpThreads[threadIndex] is nil or when frankenPHPContext() returns nil. Consider adding a test that calls the logger functions with an invalid or out-of-bounds threadIndex to verify the fallback behavior works as expected.

Copilot uses AI. Check for mistakes.

lobre mentioned this pull request

Dec 19, 2025

Closing in favor of #2089.