Skip to content

fix(log): don't discard bytes >4096 in FileHandler - #4415

Merged
iuioiua merged 8 commits into
denoland:mainfrom
syhol:fix-error-on-large-logs
Mar 1, 2024
Merged

fix(log): don't discard bytes >4096 in FileHandler#4415
iuioiua merged 8 commits into
denoland:mainfrom
syhol:fix-error-on-large-logs

Conversation

@syhol

@syhol syhol commented Feb 29, 2024

Copy link
Copy Markdown
Contributor

Fixes the bug #4411
Bug introduced in #4021

TLDR; Attempting to log 4096 or more bytes in a single call to the logger would throw an error and the log would not be written.

Note this PR appears to introduce a new dependency between std/log and std/io. If this is not acceptable then we can find a workaround but the solution will be more verbose.

@syhol
syhol requested a review from kt3k as a code owner February 29, 2024 00:08
@github-actions github-actions Bot added the log label Feb 29, 2024
@syhol

syhol commented Feb 29, 2024

Copy link
Copy Markdown
Contributor Author

I'm not sure why canary is failing, I don't think it's related to my change as its failing in ./testing/snapshot_test.ts and I don't see any link between that and the log module.

@iuioiua iuioiua changed the title fix(log): error logging >4096 bytes in FileHandler/RotatingFileHandler fix(log): don't discard bytes >4096 in FileHandler Feb 29, 2024
Comment thread log/file_handler_test.ts Outdated
Comment thread log/file_handler_test.ts Outdated
Comment thread log/rotating_file_handler_test.ts Outdated
Comment thread log/rotating_file_handler_test.ts Outdated
@kt3k

kt3k commented Feb 29, 2024

Copy link
Copy Markdown
Contributor

Note this PR appears to introduce a new dependency between std/log and std/io. If this is not acceptable then we can find a workaround but the solution will be more verbose.

We recently slightly changed the policy about std/io (ref: #4128), and now it's not a deprecated module anymore. So the dependency to std/io is just fine.

syhol and others added 6 commits February 29, 2024 09:42
Co-authored-by: Asher Gomez <ashersaupingomez@gmail.com>
Co-authored-by: Asher Gomez <ashersaupingomez@gmail.com>
Co-authored-by: Asher Gomez <ashersaupingomez@gmail.com>
Co-authored-by: Asher Gomez <ashersaupingomez@gmail.com>
@syhol
syhol requested a review from iuioiua February 29, 2024 13:45
@syhol

syhol commented Feb 29, 2024

Copy link
Copy Markdown
Contributor Author

Looks like #4416 fixed the canary issues, thanks for that @kt3k 🙏

@iuioiua's suggestions have been merged and it's ready for another review.

@kt3k kt3k left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Thanks!

@iuioiua

iuioiua commented Mar 1, 2024

Copy link
Copy Markdown
Contributor

Thank you, @syhol!

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants