Skip to content

fix(logger): print to stderr if log fails for default format#2830

Merged
ReneWerner87 merged 1 commit intogofiber:mainfrom
nickajacks1:logger-errcheck
Feb 5, 2024
Merged

fix(logger): print to stderr if log fails for default format#2830
ReneWerner87 merged 1 commit intogofiber:mainfrom
nickajacks1:logger-errcheck

Conversation

@nickajacks1
Copy link
Member

Description

We log to stderr if logging fails when a custom format is used, but not for the default format. This change addresses this inconsistency.

Type of Change

Please delete options that are not relevant.

  • Enhancement (improvement to existing features and functionality)
  • Code consistency (non-breaking change which improves code reliability and robustness)

Checklist

Before you submit your pull request, please make sure you meet these requirements:

  • Conducted a self-review of the code and provided comments for complex or critical parts.
  • Ensured that new and existing unit tests pass locally with the changes.

We log to stderr if logging fails when a custom format is used, but not
for the default format. This change addresses this inconsistency.
@nickajacks1
Copy link
Member Author

One other thing I noticed was that the log write is only protected by a mutex lock when the non-default format is used. Perhaps we should be locking in both places...?

@efectn
Copy link
Member

efectn commented Feb 4, 2024

One other thing I noticed was that the log write is only protected by a mutex lock when the non-default format is used. Perhaps we should be locking in both places...?

There should not be issue when writing logs to a file using POSIX write() or stdout. But there are probably some edge cases that may cause data races. So i'd be OK to add mutex both places.

@nickajacks1
Copy link
Member Author

Thought about the locking some more. It's probably not a good idea for fiber to add locking. Since os.File operations are goroutine safe, and since I'm sure >99% of cases will be logging to os.File, it's probably better to have users implement locking in their Write methods. Either way, I'll follow up with a separate PR later.

@ReneWerner87 ReneWerner87 added the v3 label Feb 5, 2024
@ReneWerner87 ReneWerner87 merged commit 926c537 into gofiber:main Feb 5, 2024
@nickajacks1 nickajacks1 deleted the logger-errcheck branch February 5, 2024 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants