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(log): don't discard bytes >4096 in FileHandler #4415

Merged
merged 8 commits into from
Mar 1, 2024

Conversation

syhol
Copy link
Contributor

@syhol syhol commented Feb 29, 2024

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
Copy link
Contributor Author

syhol commented Feb 29, 2024

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
log/file_handler_test.ts Outdated Show resolved Hide resolved
log/file_handler_test.ts Outdated Show resolved Hide resolved
log/rotating_file_handler_test.ts Outdated Show resolved Hide resolved
log/rotating_file_handler_test.ts Outdated Show resolved Hide resolved
@kt3k
Copy link
Member

kt3k commented Feb 29, 2024

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
Copy link
Contributor Author

syhol commented Feb 29, 2024

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

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

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@iuioiua
Copy link
Contributor

iuioiua commented Mar 1, 2024

Thank you, @syhol!

@iuioiua iuioiua merged commit e5e737b into denoland:main Mar 1, 2024
7 checks passed
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