Bug 236807 – [DataLog] Disable buffering of output only for file, not stderr

• Component: New Bugs
• Status: ASSIGNED

[Basuke Suzuki]

Stderr is usually unbuffered. This requires only for file.

[Basuke Suzuki]

Created attachment 452440 [details]
PATCH

[Aakash Jain]

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.

[Yusuke Suzuki]

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.

[Basuke Suzuki]

(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.

[Basuke Suzuki]

I'll do this by wrapping with PLATFORM macro.

[Don Olmstead]

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.

[Basuke Suzuki]

Thanks. Platform dependent macro sounds reasonable for this.

[Fujii Hironori]

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.

[Darin Adler]

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.

[Radar WebKit Bug Importer]

<rdar://problem/89382520>

[Basuke Suzuki]

> 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.