Summary: | [DataLog] Disable buffering of output only for file, not stderr | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Basuke Suzuki <basuke> | ||||
Component: | New Bugs | Assignee: | Basuke Suzuki <basuke> | ||||
Status: | ASSIGNED --- | ||||||
Severity: | Normal | CC: | aakash_jain, achristensen, basuke, benjamin, cdumez, cmarcelo, darin, don.olmstead, ews-watchlist, Hironori.Fujii, msaboff, webkit-bug-importer, ysuzuki | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Basuke Suzuki
2022-02-17 15:26:57 PST
Created attachment 452440 [details]
PATCH
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 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. (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. I'll do this by wrapping with PLATFORM macro. 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. Thanks. Platform dependent macro sounds reasonable for this. 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 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.
> 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.
|