Re: testdup test fails when compiled in debug mode on Windows
On Thu, Nov 7, 2013 at 11:20 AM, William A. Rowe Jr. wrote: > On Thu, 7 Nov 2013 09:43:07 -0500 > Jeff Trawick wrote: > > > On Thu, Nov 7, 2013 at 12:33 AM, William A. Rowe Jr. > > wrote: > > > > > On Wed, 06 Nov 2013 16:07:37 -0800 > > > Mike Rumph wrote: > > > > > > > > > > > On 11/6/2013 1:06 PM, Jeff Trawick wrote: > > > > > > > > > > I just played with _commit() on stdin a bit. It turns out that > > > > > _commit(0) fails if stdin is redirected (main.exe < somefile) > > > > > but works if stdin is a tty. That's the opposite of _commit(1 > > > > > or 2). But I don't see how _commit(0) makes sense anyway, so I > > > > > simply removed the call instead of reversing the corresponding > > > > > _isatty() check in your patch. > > > > > > > > > > trunk: r1539455 > > > > > 1.5.x branch: r1539461 > > > > Okay Jeff, > > > > > > > > I just tried both stdout and stdin, and got the same results that > > > > you did. Strange but true. > > > > > > IIRC the choice to _commit ahead of any handling of stdin/out/err > > > reflected the possibility that bytes were queued/stuck in the FILE > > > or the msvcrt 'fd' (not really an fd at all) before assuming > > > ownership of the file handle. It might have been an overreaction > > > to a problem that wouldn't exist in practice. But I'd prefer if > > > this were left context sensitive to _DEBUG mode builds. > > > > > > > "The *_commit* function forces the operating system to write the file > > associated with *handle* to disk" > > > > _commit(fileno_stdout or fileno_stderr) fails if it refers to the > > terminal whether or not it is a debug build. It simply isn't a valid > > call. I called out the debug build issue in CHANGES because that is > > likely the only case where anyone would encounter a problem symptom. > > (_commit() otherwise continues to return -1/EBADF and nobody notices.) > > > > The fileno_stdin issue is even more odd as it took an opposite sense > > of _isatty(fileno_stdin) to keep it from reporting an error, but I > > don't see any connection between _commit() and stdin so it seemed > > most appropriate to simply remove the call for stdin. > > > > IOW, I don't see the need to tie this to debug builds because the > > calls are invalid whether or not the RTL pops up the dialog. > > In that case I can see the benefit to dropping the call entirely. > Maybe we've lost communication here... The calls that are invalid are, essentially, _commit(console), which we've filtered out via a check to _isatty(). _commit(file) still makes sense AFAIK and still is used. > > On Thu, 07 Nov 2013 07:09:18 -0800 > Mike Rumph wrote: > > > Do you remember if the reasons for using _commit() would distinguish > > between input and output? > > No, not offhand. In any case, they were in inverted order to clean up > any lingering input ahead of dup'ing or creating an apr file_t from an > apr_os_file_t. > > Let's axe it. > > -- Born in Roswell... married an alien... http://emptyhammock.com/
Re: testdup test fails when compiled in debug mode on Windows
On Thu, 7 Nov 2013 09:43:07 -0500 Jeff Trawick wrote: > On Thu, Nov 7, 2013 at 12:33 AM, William A. Rowe Jr. > wrote: > > > On Wed, 06 Nov 2013 16:07:37 -0800 > > Mike Rumph wrote: > > > > > > > > On 11/6/2013 1:06 PM, Jeff Trawick wrote: > > > > > > > > I just played with _commit() on stdin a bit. It turns out that > > > > _commit(0) fails if stdin is redirected (main.exe < somefile) > > > > but works if stdin is a tty. That's the opposite of _commit(1 > > > > or 2). But I don't see how _commit(0) makes sense anyway, so I > > > > simply removed the call instead of reversing the corresponding > > > > _isatty() check in your patch. > > > > > > > > trunk: r1539455 > > > > 1.5.x branch: r1539461 > > > Okay Jeff, > > > > > > I just tried both stdout and stdin, and got the same results that > > > you did. Strange but true. > > > > IIRC the choice to _commit ahead of any handling of stdin/out/err > > reflected the possibility that bytes were queued/stuck in the FILE > > or the msvcrt 'fd' (not really an fd at all) before assuming > > ownership of the file handle. It might have been an overreaction > > to a problem that wouldn't exist in practice. But I'd prefer if > > this were left context sensitive to _DEBUG mode builds. > > > > "The *_commit* function forces the operating system to write the file > associated with *handle* to disk" > > _commit(fileno_stdout or fileno_stderr) fails if it refers to the > terminal whether or not it is a debug build. It simply isn't a valid > call. I called out the debug build issue in CHANGES because that is > likely the only case where anyone would encounter a problem symptom. > (_commit() otherwise continues to return -1/EBADF and nobody notices.) > > The fileno_stdin issue is even more odd as it took an opposite sense > of _isatty(fileno_stdin) to keep it from reporting an error, but I > don't see any connection between _commit() and stdin so it seemed > most appropriate to simply remove the call for stdin. > > IOW, I don't see the need to tie this to debug builds because the > calls are invalid whether or not the RTL pops up the dialog. In that case I can see the benefit to dropping the call entirely. On Thu, 07 Nov 2013 07:09:18 -0800 Mike Rumph wrote: > Do you remember if the reasons for using _commit() would distinguish > between input and output? No, not offhand. In any case, they were in inverted order to clean up any lingering input ahead of dup'ing or creating an apr file_t from an apr_os_file_t. Let's axe it.
Re: testdup test fails when compiled in debug mode on Windows
Hello Bill, That is a very good point, since the assert box will only appear under debug mode. But when not in debug mode _commit(2) will send back a "-1" return code and set errno to EBADF, if stderr is not redirected to a file. The error existed in both cases, but the apr_file_dup2() function in apr/file_io/win32/filedup.c was simply ignoring the return code from _commit(). Perhaps it should be checked. There is also the issue that Jeff uncovered for stdin having a reversed effect from stdout and stderr. Do you remember if the reasons for using _commit() would distinguish between input and output? We should probably also consider whether or not the fflush(stdin) call makes sense as well, dependent on whether or not stdin is redirected or piped in from another process. Thanks, Mike Rumph On 11/6/2013 9:33 PM, William A. Rowe Jr. wrote: On Wed, 06 Nov 2013 16:07:37 -0800 Mike Rumph wrote: On 11/6/2013 1:06 PM, Jeff Trawick wrote: I just played with _commit() on stdin a bit. It turns out that _commit(0) fails if stdin is redirected (main.exe < somefile) but works if stdin is a tty. That's the opposite of _commit(1 or 2). But I don't see how _commit(0) makes sense anyway, so I simply removed the call instead of reversing the corresponding _isatty() check in your patch. trunk: r1539455 1.5.x branch: r1539461 Okay Jeff, I just tried both stdout and stdin, and got the same results that you did. Strange but true. IIRC the choice to _commit ahead of any handling of stdin/out/err reflected the possibility that bytes were queued/stuck in the FILE or the msvcrt 'fd' (not really an fd at all) before assuming ownership of the file handle. It might have been an overreaction to a problem that wouldn't exist in practice. But I'd prefer if this were left context sensitive to _DEBUG mode builds.
Re: testdup test fails when compiled in debug mode on Windows
On Thu, Nov 7, 2013 at 12:33 AM, William A. Rowe Jr. wrote: > On Wed, 06 Nov 2013 16:07:37 -0800 > Mike Rumph wrote: > > > > > On 11/6/2013 1:06 PM, Jeff Trawick wrote: > > > > > > I just played with _commit() on stdin a bit. It turns out that > > > _commit(0) fails if stdin is redirected (main.exe < somefile) but > > > works if stdin is a tty. That's the opposite of _commit(1 or 2). > > > But I don't see how _commit(0) makes sense anyway, so I simply > > > removed the call instead of reversing the corresponding _isatty() > > > check in your patch. > > > > > > trunk: r1539455 > > > 1.5.x branch: r1539461 > > Okay Jeff, > > > > I just tried both stdout and stdin, and got the same results that you > > did. Strange but true. > > IIRC the choice to _commit ahead of any handling of stdin/out/err > reflected the possibility that bytes were queued/stuck in the FILE or > the msvcrt 'fd' (not really an fd at all) before assuming ownership of > the file handle. It might have been an overreaction to a problem that > wouldn't exist in practice. But I'd prefer if this were left context > sensitive to _DEBUG mode builds. > "The *_commit* function forces the operating system to write the file associated with *handle* to disk" _commit(fileno_stdout or fileno_stderr) fails if it refers to the terminal whether or not it is a debug build. It simply isn't a valid call. I called out the debug build issue in CHANGES because that is likely the only case where anyone would encounter a problem symptom. (_commit() otherwise continues to return -1/EBADF and nobody notices.) The fileno_stdin issue is even more odd as it took an opposite sense of _isatty(fileno_stdin) to keep it from reporting an error, but I don't see any connection between _commit() and stdin so it seemed most appropriate to simply remove the call for stdin. IOW, I don't see the need to tie this to debug builds because the calls are invalid whether or not the RTL pops up the dialog. -- Born in Roswell... married an alien... http://emptyhammock.com/
Re: testdup test fails when compiled in debug mode on Windows
On Wed, 06 Nov 2013 16:07:37 -0800 Mike Rumph wrote: > > On 11/6/2013 1:06 PM, Jeff Trawick wrote: > > > > I just played with _commit() on stdin a bit. It turns out that > > _commit(0) fails if stdin is redirected (main.exe < somefile) but > > works if stdin is a tty. That's the opposite of _commit(1 or 2). > > But I don't see how _commit(0) makes sense anyway, so I simply > > removed the call instead of reversing the corresponding _isatty() > > check in your patch. > > > > trunk: r1539455 > > 1.5.x branch: r1539461 > Okay Jeff, > > I just tried both stdout and stdin, and got the same results that you > did. Strange but true. IIRC the choice to _commit ahead of any handling of stdin/out/err reflected the possibility that bytes were queued/stuck in the FILE or the msvcrt 'fd' (not really an fd at all) before assuming ownership of the file handle. It might have been an overreaction to a problem that wouldn't exist in practice. But I'd prefer if this were left context sensitive to _DEBUG mode builds.
Re: testdup test fails when compiled in debug mode on Windows
On 11/6/2013 1:06 PM, Jeff Trawick wrote: I just played with _commit() on stdin a bit. It turns out that _commit(0) fails if stdin is redirected (main.exe < somefile) but works if stdin is a tty. That's the opposite of _commit(1 or 2). But I don't see how _commit(0) makes sense anyway, so I simply removed the call instead of reversing the corresponding _isatty() check in your patch. trunk: r1539455 1.5.x branch: r1539461 Okay Jeff, I just tried both stdout and stdin, and got the same results that you did. Strange but true. -- Born in Roswell... married an alien... http://emptyhammock.com/
Re: testdup test fails when compiled in debug mode on Windows
On Wed, Nov 6, 2013 at 2:13 PM, Jeff Trawick wrote: > On Wed, Nov 6, 2013 at 2:06 PM, Mike Rumph wrote: > >> The apr_file_dup2() function in apr/file_io/win32/filedup.c calls >> _commit() for standard file handles 0, 1 and 2. >> The _commit() function will assert with the message "Invalid file >> descriptor. File possibly closed by a different thread" or return a value >> of -1 if the file handle refers to a device. >> The assert will appear if APR is compiled in debug mode. >> >> This can be seen by running "testall.exe testdup". >> But if file redirection is used (such as "testall.exe testdup > outfile >> 2>&1"), then the test completes successfully. >> >> I have attached a patch that corrects this problem by checking _isatty(). >> >> Credit goes to Jeff Trawick for catching this problem in the first place. >> > > Well, there's a lot of unhappiness when you have to answer the > abort/retry/ignore dialog when you run httpd with a debug flavor of apr ;) > > >> Thanks, >> >> Mike Rumph >> > > The existing flush-stdin path sure looks funny now that it is highlighted > by the format of the patch, but that's not related to this bug and google > seems to think that it is acceptable on Windows. > > I hope to commit shortly. Thanks! > > -- > Born in Roswell... married an alien... > http://emptyhammock.com/ > I just played with _commit() on stdin a bit. It turns out that _commit(0) fails if stdin is redirected (main.exe < somefile) but works if stdin is a tty. That's the opposite of _commit(1 or 2). But I don't see how _commit(0) makes sense anyway, so I simply removed the call instead of reversing the corresponding _isatty() check in your patch. trunk: r1539455 1.5.x branch: r1539461 -- Born in Roswell... married an alien... http://emptyhammock.com/
Re: testdup test fails when compiled in debug mode on Windows
On Wed, Nov 6, 2013 at 2:06 PM, Mike Rumph wrote: > The apr_file_dup2() function in apr/file_io/win32/filedup.c calls > _commit() for standard file handles 0, 1 and 2. > The _commit() function will assert with the message "Invalid file > descriptor. File possibly closed by a different thread" or return a value > of -1 if the file handle refers to a device. > The assert will appear if APR is compiled in debug mode. > > This can be seen by running "testall.exe testdup". > But if file redirection is used (such as "testall.exe testdup > outfile > 2>&1"), then the test completes successfully. > > I have attached a patch that corrects this problem by checking _isatty(). > > Credit goes to Jeff Trawick for catching this problem in the first place. > Well, there's a lot of unhappiness when you have to answer the abort/retry/ignore dialog when you run httpd with a debug flavor of apr ;) > Thanks, > > Mike Rumph > The existing flush-stdin path sure looks funny now that it is highlighted by the format of the patch, but that's not related to this bug and google seems to think that it is acceptable on Windows. I hope to commit shortly. Thanks! -- Born in Roswell... married an alien... http://emptyhammock.com/
