Skip to content

zapcore: recover panics from ObjectMarshaler and ArrayMarshaler (#1504)#1528

Open
rodionsteshenko wants to merge 1 commit intouber-go:masterfrom
rodionsteshenko:fix-issue-1504
Open

zapcore: recover panics from ObjectMarshaler and ArrayMarshaler (#1504)#1528
rodionsteshenko wants to merge 1 commit intouber-go:masterfrom
rodionsteshenko:fix-issue-1504

Conversation

@rodionsteshenko
Copy link

Fixes #1504

Problem

Panics in user-provided MarshalLogObject and MarshalLogArray implementations crash the entire application. This is inconsistent with the existing panic recovery for Stringer (encodeStringer) and Error types, which gracefully convert panics to PANIC=<msg> error entries.

Fix

Added three panic-recovering wrapper functions (encodeObject, encodeArray, encodeInlinedObject) that mirror the existing encodeStringer pattern. These wrap the calls to enc.AddObject, enc.AddArray, and obj.MarshalLogObject respectively, converting any panic to a PANIC=<msg> error that gets logged as a <key>Error field.

Affected field types:

  • ObjectMarshalerType
  • ArrayMarshalerType
  • InlineMarshalerType

Test

Added panickyObject and panickyArray test types plus 3 new test cases to TestFieldAddingError that verify:

  • A panicking ObjectMarshaler does not crash, produces PANIC=panic in MarshalLogObject
  • A panicking ArrayMarshaler does not crash, produces PANIC=panic in MarshalLogArray
  • A panicking inlined ObjectMarshaler does not crash, produces PANIC=panic in MarshalLogObject

Full test suite passes on macOS ARM (Apple Silicon).

…ineMarshalerType

Fixes uber-go#1504

Previously, panics in user-provided MarshalLogObject and MarshalLogArray
implementations would crash the entire application. This is inconsistent
with the existing panic recovery for Stringer (encodeStringer) and Error
types.

This change wraps ObjectMarshalerType, ArrayMarshalerType, and
InlineMarshalerType field additions with panic recovery, converting
panics to PANIC=<msg> error entries in the log context, consistent
with the existing behavior for StringerType.
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Badly coded MarshalLogObject can panic zap

2 participants