Skip to content

fix: make the logs line buffered #9496

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

Merged
merged 2 commits into from
Jul 5, 2021
Merged

Conversation

mahdifrmz
Copy link
Contributor

fixes #9432
Not quite sure if that's what you want, but storing the log message in buffers eliminated the tiny write syscalls for me.
I just changed the Logger struct.

@lnicola
Copy link
Member

lnicola commented Jul 5, 2021

Oh, I didn't even realize that was our logging sink. I'm surprised it behaves like this, since it's already using a BufWriter.

Anyway, #9274 will probably get rid or replace that code.

@lnicola
Copy link
Member

lnicola commented Jul 5, 2021

Oh, it's stderr that's not buffered.

@mahdifrmz
Copy link
Contributor Author

Yeah, I actually checked stderr, I think that's the main issue.

@matklad
Copy link
Member

matklad commented Jul 5, 2021

TIL: rust-lang/rust#64413

Let's just quickly fix this by formatting the string to memory with format, and then writing the whole thing to stderr in one go.

@lnicola
Copy link
Member

lnicola commented Jul 5, 2021

Can we wrap stderr in a LineWriter or BufWriter and stick it in self?

@mahdifrmz
Copy link
Contributor Author

Let's just quickly fix this by formatting the string to memory with format, and then writing the whole thing to stderr in one go.

I've done the same for the Logger in this PR. or you mean for the whole source code?

self.no_buffering

if self.no_buffering {
self.flush();
Copy link
Member

@lnicola lnicola Jul 5, 2021

Choose a reason for hiding this comment

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

Suggested change
self.flush();
w.lock().flush();

(or better, hoist that to a local).

Then flush is dead code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well we can't remove flush as it's part of Log trait.
Instead, I will put the actual flush inside of it.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, sorry, I missed that.

Copy link
Member

Choose a reason for hiding this comment

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

Not to be too nit-picky, but this feels strange -- what if log calls flush internally?

I would have left flush unchanged, moved w.lock() to a variable, then called flush on that in this branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess what you're suggesting makes sense, that way we had to acquire the lock twice.

self.no_buffering

if self.no_buffering {
writer.flush().unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Maybe

Suggested change
writer.flush().unwrap();
let _ = writer.flush();

But I'm not sure how I feel about it. This might cause some spurious panics when we're run by another program and it closes our stderr.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I only put the unwrap() to get rid of the warning. thanks for mentioning.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, then maybe let's ignore the errors here.

@lnicola
Copy link
Member

lnicola commented Jul 5, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Jul 5, 2021

@bors bors bot merged commit 91bfa4b into rust-lang:master Jul 5, 2021
@mahdifrmz mahdifrmz deleted the log-line-buffer branch July 14, 2021 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make sure logs are at least line-buffered
3 participants