Re: [PATCH] mingw: make stderr unbuffered again

Johannes Schindelin [off-list ref] writes:

When removing the hack for isatty(), we actually removed more than just
an isatty() hack: we removed the hack where internal data structures of
the MSVC runtime are modified in order to redirect stdout/stderr.

Instead of using that hack (that does not work with newer versions of
the runtime, anyway), we replaced it by reopening the respective file
descriptors.

What we forgot was to mark stderr as unbuffered again.

Reported by Hannes Sixt. Fixed with Jeff Hostetler's assistance.

Signed-off-by: Johannes Schindelin <redacted>
---
Published-As: https://github.com/dscho/git/releases/tag/mingw-unbuffered-stderr-v1
Fetch-It-Via: git fetch https://github.com/dscho/git mingw-unbuffered-stderr-v1

OK.  Should this go directly to 'master', as the isatty thing is
already in?

Hi Junio,

On Mon, 13 Feb 2017, Junio C Hamano wrote:

Johannes Schindelin [off-list ref] writes:

quoted

When removing the hack for isatty(), we actually removed more than just
an isatty() hack: we removed the hack where internal data structures of
the MSVC runtime are modified in order to redirect stdout/stderr.

Instead of using that hack (that does not work with newer versions of
the runtime, anyway), we replaced it by reopening the respective file
descriptors.

What we forgot was to mark stderr as unbuffered again.

Reported by Hannes Sixt. Fixed with Jeff Hostetler's assistance.

Signed-off-by: Johannes Schindelin <redacted>
---
Published-As: https://github.com/dscho/git/releases/tag/mingw-unbuffered-stderr-v1
Fetch-It-Via: git fetch https://github.com/dscho/git mingw-unbuffered-stderr-v1

OK.  Should this go directly to 'master', as the isatty thing is
already in?

From my point of view, it is not crucial. The next Git for Windows version
will have it, of course, and Hannes is always running with his set of
patches, he can easily cherry-pick this one, too.

Ciao,
Johannes

P.S.: Could you please cut the remainder of the mail that you are not
responding to? Thanks.

Am 14.02.2017 um 15:47 schrieb Johannes Schindelin:

On Mon, 13 Feb 2017, Junio C Hamano wrote:

quoted

Johannes Schindelin [off-list ref] writes:

quoted

When removing the hack for isatty(), we actually removed more than just
an isatty() hack: we removed the hack where internal data structures of
the MSVC runtime are modified in order to redirect stdout/stderr.

Instead of using that hack (that does not work with newer versions of
the runtime, anyway), we replaced it by reopening the respective file
descriptors.

What we forgot was to mark stderr as unbuffered again.

I do not see how the earlier patch turned stderr from unbuffered to buffered, as it did not add or remove any setvbuf() call. Can you explain?

quoted

OK.  Should this go directly to 'master', as the isatty thing is
already in?

quoted

From my point of view, it is not crucial. The next Git for Windows version

will have it, of course, and Hannes is always running with his set of
patches, he can easily cherry-pick this one, too.

The patch fixes the problem for me here.

I am a bit hesitant to call it "not crucial": The symptom I observed is certainly not a show stopper; but who knows where else it is important that stderr goes to the terminal earlier than other output. As it is a new regression, it should be included in the next release.

Thanks,
-- Hannes

Hi Hannes,

On Tue, 14 Feb 2017, Johannes Sixt wrote:

Am 14.02.2017 um 15:47 schrieb Johannes Schindelin:

quoted

On Mon, 13 Feb 2017, Junio C Hamano wrote:

quoted

Johannes Schindelin [off-list ref] writes:

quoted

When removing the hack for isatty(), we actually removed more than
just an isatty() hack: we removed the hack where internal data
structures of the MSVC runtime are modified in order to redirect
stdout/stderr.

Instead of using that hack (that does not work with newer versions
of the runtime, anyway), we replaced it by reopening the
respective file descriptors.

What we forgot was to mark stderr as unbuffered again.

I do not see how the earlier patch turned stderr from unbuffered to
buffered, as it did not add or remove any setvbuf() call. Can you
explain?

Certainly. I hoped that the commit message would do the job, but then, I
am under time pressure these days, so it was probably a bit terse.

The hack we used to make isatty() work used to change data structures
directly that are *internal* to the MSVC runtime. That is, instead of
*really* redirecting stdout/stderr, we simply changed the target of the
existing stdout/stderr and thereby could fool MSVC into believing that it
is *still* writing to the terminal (because the bit is set) and that it is
*not* a pipe (because we forcibly unset that bit).

Needless to say, this meddling with internal data structures is not only
prone to break with future updates of the MSVC runtime, it is also
inappropriate because the implementation may rely on certain side effects
(or not so side effects) that may very well cause crashes or data loss.
Imagine, for example, that the internal data structure were variable-size,
based on the HANDLE type. That is totally legitimate for an internal data
structure. And if we meddle with the HANDLE, we can easily cause data
corruption.

As GCC is basically tied to using an older version of the MSVC runtime
(unlike GLIBC, multiple versions of the MSVC runtime can coexist happily,
making it relatively easy to maintain backwards-compatibility, but that
concept is foreign to GCC), this used to not be a problem.

However, with our recent push to allow developing, building, debugging and
performance profiling in Visual Studio, that limitation no longer holds
true: if you develop with Visual Studio 2015, you will link to a newer
MSVC runtime, and that runtime changed those internal data structures
rather dramatically.

That means that we had to come up with a non-hacky way to redirect
stdout/stderr (e.g. to parse ESC color sequences and apply them to the
Win32 Console as appropriate) and still have isatty() report what we want
it to report. That is, if we redirect stdout/stderr to a pipe that parses
the color sequences for use in the Win32 Console, we want isatty() to
report that we are writing to a Terminal Typewriter (a charming
anachronism, isn't it?).

My colleague Jeff Hostetler worked on this and figured out that the only
way to do this properly is to wrap isatty() and override it via a #define,
and simply remember when stdout/stderr referred to a terminal before
redirecting, say, to that pipe.

As my famously broken patch to override isatty() (so that /dev/null would
not be treated as a terminal) *already* overrode isatty() with a custom
wrapper, and as it was broken and needed fixing, I decided to reconcile
the two approaches into what is now the version in `master`.

So instead of "bending" the target HANDLE of the existing stdout/stderr
(which would *naturally* have kept the buffered/unbuffered nature as-is),
we now redirect with correct API calls. And the patch I provided at the
bottom of this mail thread reinstates the unbuffered nature of stderr now
that it gets reopened.

Hopefully that makes it clear why the setvbuf() call is required now, but
was previously unnecessary?

Ciao,
Dscho

Am 15.02.2017 um 13:32 schrieb Johannes Schindelin:

On Tue, 14 Feb 2017, Johannes Sixt wrote:

quoted

Am 14.02.2017 um 15:47 schrieb Johannes Schindelin:

quoted

On Mon, 13 Feb 2017, Junio C Hamano wrote:

quoted

Johannes Schindelin [off-list ref] writes:

quoted

What we forgot was to mark stderr as unbuffered again.

I do not see how the earlier patch turned stderr from unbuffered to
buffered, as it did not add or remove any setvbuf() call. Can you
explain?

[ motivation and history of js/mingw-isatty snipped ]

So instead of "bending" the target HANDLE of the existing stdout/stderr
(which would *naturally* have kept the buffered/unbuffered nature as-is),
we now redirect with correct API calls.

Your statement implies that at the time when winansi_init() begins, stdio is already initialized and the buffered/unbuffered state has been set for stderr. I would think that this is true.

Then we swap out the file handle underlying stderr in swap_osfhnd() using dup2(). Why would that change the buffered state of stdio?

And the patch I provided at the
bottom of this mail thread reinstates the unbuffered nature of stderr now
that it gets reopened.

Hopefully that makes it clear why the setvbuf() call is required now, but
was previously unnecessary?

Unfortunately, no. I do not see how dup2() causes a change in stdio state. I must be missing something (and that may be a basic misunderstanding of how stdio is initialized).

-- Hannes

Hi Hannes,

On Wed, 15 Feb 2017, Johannes Sixt wrote:

Am 15.02.2017 um 13:32 schrieb Johannes Schindelin:

quoted

On Tue, 14 Feb 2017, Johannes Sixt wrote:

quoted

Am 14.02.2017 um 15:47 schrieb Johannes Schindelin:

quoted

On Mon, 13 Feb 2017, Junio C Hamano wrote:

quoted

Johannes Schindelin [off-list ref] writes:

quoted

What we forgot was to mark stderr as unbuffered again.

I do not see how the earlier patch turned stderr from unbuffered to
buffered, as it did not add or remove any setvbuf() call. Can you
explain?

[ motivation and history of js/mingw-isatty snipped ]

So instead of "bending" the target HANDLE of the existing
stdout/stderr (which would *naturally* have kept the
buffered/unbuffered nature as-is), we now redirect with correct API
calls.

Your statement implies that at the time when winansi_init() begins,
stdio is already initialized and the buffered/unbuffered state has been
set for stderr.  I would think that this is true.

Then we swap out the file handle underlying stderr in swap_osfhnd()
using dup2(). Why would that change the buffered state of stdio?

The file handle we swap in for stderr points to the pipe that a
freshly-started thread consumes for parsing the ANSI color sequences. This
handle is used both for stdout and stderr. The dup2() call then implicitly
reopens stderr, with the default buffering.

quoted

And the patch I provided at the bottom of this mail thread reinstates
the unbuffered nature of stderr now that it gets reopened.

Hopefully that makes it clear why the setvbuf() call is required now,
but was previously unnecessary?

Unfortunately, no. I do not see how dup2() causes a change in stdio state. I
must be missing something (and that may be a basic misunderstanding of how
stdio is initialized).

It appears that dup2()ing fd 2 resets that stdio state.

Ciao,
Dscho

Am 16.02.2017 um 18:10 schrieb Johannes Schindelin:

On Wed, 15 Feb 2017, Johannes Sixt wrote:

quoted

I do not see how dup2() causes a change in stdio state. I
must be missing something (and that may be a basic misunderstanding of how
stdio is initialized).

It appears that dup2()ing fd 2 resets that stdio state.

OK, thanks. It's surprising, but so be it.

Thank you for the quick fix!

-- Hannes