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(logger): print to stderr if log fails for default format #2830

Merged
merged 1 commit into from
Feb 5, 2024

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
12 checks passed
@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