Bug 236807 - [DataLog] Disable buffering of output only for file, not stderr
Summary: [DataLog] Disable buffering of output only for file, not stderr
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Basuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-02-17 15:26 PST by Basuke Suzuki
Modified: 2022-02-25 17:25 PST (History)
13 users (show)

See Also:


Attachments
PATCH (1.65 KB, patch)
2022-02-17 15:32 PST, Basuke Suzuki
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Basuke Suzuki 2022-02-17 15:26:57 PST
Stderr is usually unbuffered. This requires only for file.
Comment 1 Basuke Suzuki 2022-02-17 15:32:56 PST
Created attachment 452440 [details]
PATCH
Comment 2 Aakash Jain 2022-02-18 07:18:08 PST
I cancelled https://ews-build.webkit.org/#/builders/68/builds/8529 to speed up ios-wk2 queue. It already finished running layout-tests and the failures seems to be pre-existing.
Comment 3 Yusuke Suzuki 2022-02-21 17:32:58 PST
Comment on attachment 452440 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=452440&action=review

> Source/WTF/ChangeLog:8
> +        `stderr` is usually already unbuffered. Disabling buffering is required only for file.

I have two questions :)

1. Is it guaranteed that `stderr` is unbuffered? (e.g. specified in C spec etc.).
2. What is the benefit of doing this only for file? If (1) is not guaranteed, always calling setvbuf sounds safer option to me, so I would like to know the motivation.
Comment 4 Basuke Suzuki 2022-02-21 17:39:37 PST
(In reply to Yusuke Suzuki from comment #3)
> Comment on attachment 452440 [details]
> PATCH
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=452440&action=review
> 
> > Source/WTF/ChangeLog:8
> > +        `stderr` is usually already unbuffered. Disabling buffering is required only for file.
> 
> I have two questions :)
> 
> 1. Is it guaranteed that `stderr` is unbuffered? (e.g. specified in C spec
> etc.).
> 2. What is the benefit of doing this only for file? If (1) is not
> guaranteed, always calling setvbuf sounds safer option to me, so I would
> like to know the motivation.

1. I use the word "usually" casually without doing a wide research, but as far as I do quick googling, Linux, FreeBSD do disabling buffering by default. If not so, there might be the reason not to do that. stderr is something provided from the system so that it is better to use it as is.

2. Above is the motivation. Not changing the system's expectation is the benefit.
Comment 5 Basuke Suzuki 2022-02-21 18:07:21 PST
I'll do this by wrapping with PLATFORM macro.
Comment 6 Don Olmstead 2022-02-22 10:43:33 PST
Comment on attachment 452440 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=452440&action=review

>>> Source/WTF/ChangeLog:8
>>> +        `stderr` is usually already unbuffered. Disabling buffering is required only for file.
>> 
>> I have two questions :)
>> 
>> 1. Is it guaranteed that `stderr` is unbuffered? (e.g. specified in C spec etc.).
>> 2. What is the benefit of doing this only for file? If (1) is not guaranteed, always calling setvbuf sounds safer option to me, so I would like to know the motivation.
> 
> 1. I use the word "usually" casually without doing a wide research, but as far as I do quick googling, Linux, FreeBSD do disabling buffering by default. If not so, there might be the reason not to do that. stderr is something provided from the system so that it is better to use it as is.
> 
> 2. Above is the motivation. Not changing the system's expectation is the benefit.

The C standard says in 7.21.3 Files (http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2310.pdf)

At program startup, three text streams are predefined and need not be opened explicitly — standard
input (for reading conventional input), standard output (for writing conventional output), and standard
error (for writing diagnostic output). As initially opened, the standard error stream is not fully
buffered; the standard input and standard output streams are fully buffered if and only if the stream
can be determined not to refer to an interactive device.

You should definitely put more information in the CHANGELOG here rather than just a "usually". Looking at documentation does seem to provide a clear case that it is unbuffered on Linux and FreeBSD but I don't know if that's the case with Apple OS's. Windows does seem like it might be buffered https://docs.microsoft.com/en-us/cpp/c-runtime-library/stream-i-o?view=msvc-170 so some kind of platform check is probably needed for this change.
Comment 7 Basuke Suzuki 2022-02-22 11:05:37 PST
Thanks. Platform dependent macro sounds reasonable for this.
Comment 8 Fujii Hironori 2022-02-22 12:09:08 PST
In the spec:

> The setvbuf function may be used only after the stream pointed to by stream has been associated
> with an open file and before any other operation (other than an unsuccessful call to setvbuf) is
> performed on the stream. 

It seems too late to change stderr here.
Comment 9 Darin Adler 2022-02-22 15:37:15 PST
Comment on attachment 452440 [details]
PATCH

I think the patch is correct just like this and does not require platform conditionals.

I think it’s clear now that it’s an error to call setvbuf on stderr this late in the execution of a program, based on documentation of the cross platform standards that both setvbuf and stderr are part of. There is a highly-unlikely possibility that doing this to stderr is valuable on Apple platforms; someone should determine quickly, by testing, that this is not the case. Then we can land this.
Comment 10 Radar WebKit Bug Importer 2022-02-23 15:39:42 PST
<rdar://problem/89382520>
Comment 11 Basuke Suzuki 2022-02-25 17:25:31 PST
> I think it’s clear now that it’s an error to call setvbuf on stderr this
> late in the execution of a program, based on documentation of the cross
> platform standards that both setvbuf and stderr are part of. There is a
> highly-unlikely possibility that doing this to stderr is valuable on Apple
> platforms; someone should determine quickly, by testing, that this is not
> the case. Then we can land this.

Thanks Darin. Can somebody in Apple do this test? I want to add description about the effect to Apple platform.