Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fwrite to php://stderr throws E_NOTICE on php 7.4 #420

Closed
miranovy opened this issue Jul 7, 2020 · 21 comments
Closed

fwrite to php://stderr throws E_NOTICE on php 7.4 #420

miranovy opened this issue Jul 7, 2020 · 21 comments

Comments

@miranovy
Copy link

miranovy commented Jul 7, 2020

Version: 2.3.2

Bug Description

Function fwrite to php://stderr throws E_NOTICE on php 7.4. On php 7.3 and 7.2 behaves correctly.

Steps To Reproduce

<?php declare(strict_types=1);

namespace Tests;

use Tester\Assert;
use Tester\TestCase;

$container = require __DIR__ . '/../bootstrap.php';

class FwriteTest extends TestCase
{
    public function testPhpStderr()
    {
	$fw = fopen('php://stderr', 'w');
	fwrite($fw, 'same text');
	fclose($fw);
    }
}

(new FwriteTest())->run();
vendor/bin/tester -s -p php --colors 1 -C fwrite.phpt 
 _____ ___  ___ _____ ___  ___
|_   _/ __)( __/_   _/ __)| _ )
  |_| \___ /___) |_| \___ |_|_\  v2.3.2

PHP 7.4.7 (cli) | php | 8 threads

F

-- FAILED: fwrite.phpt
   Exited with error code 255 (expected 0)
   E_NOTICE: fwrite(): write of 5 bytes failed with errno=32 Broken pipe
   
   in tests/Logger/fwrite.phpt(16) 
   in [internal function]Tester\TestCase->Tester\{closure}()
   in tests/Logger/fwrite.phpt(16) fwrite()
   in src/Framework/TestCase.php(143) Tests\FwriteTest->testPhpStderr()
   in src/Framework/TestCase.php(55) Tester\TestCase->runTest()
   in tests/Logger/fwrite.phpt(22) Tester\TestCase->run()


FAILURES! (1 test, 1 failure, 0.1 seconds)

Run test separately via php fwrite.phpt behaves as expected.

same text
Error: This test forgets to execute an assertion.

Expected Behavior

Write text to stderr, same as run test separately.

@dg
Copy link
Member

dg commented Jul 7, 2020

It triggers E_NOTICE when strerr is closed since PHP 7.4. Maybe here

fclose($stderr);
or below.

@milo
Copy link
Member

milo commented Jul 19, 2020

I remember there was a problem with stderr on Windows. If stderr output was too long, test got stuck. The reason was, that Windows somehow use the same output buffer (or something like that) for stdout and stderr and PHP got stuck on reading stdout when stderr was full and vice versa. It was in PHP 5.4 times. Not sure how about PHP 7.

Beside of that. Imagine that Job catches stdin and stdout, and test produces:

echo 1 > on stdout
echo 2 > on stderr
echo 3 > on stdout
echo 4 > on stderr

we are not able to reconstruct that in Runner output, there are not output timestamps. So we get:

echo 1 > on stdout
echo 3 > on stdout
echo 2 > on stderr
echo 4 > on stderr

@dg
Copy link
Member

dg commented Jul 20, 2020

The problem with stderr also affects PHP 7.4.

@JanTvrdik
Copy link
Contributor

JanTvrdik commented Jul 20, 2020

related php/php-src@d59aac5

@MartinMystikJonas
Copy link
Contributor

I just encountered this error too with PHP 8.1. We use Nette tester to test our dev-tools. When Nette tester calls command throught symfony console it ends with this error when it outputs last line to stderr. When we run test directly then it works.

Any idea what might be the cause? It seems like stderr of test child process is for some reason closed prematurely by tester but I have no idea why.

@MartinMystikJonas
Copy link
Contributor

MartinMystikJonas commented Jul 8, 2022

Ok after some digging it seems that reson is simple. Stderr of job is closed even before it is run here (as @dg mentioned earlier):

fclose($stderr);

Is there any reson to close it? When process running in Job tries for any reson to write to stderr it will eventually produce errors.

Also RUN_COLLECT_ERRORS is useful feature and it would solve this issue in our case but I see no way how to pass this flag to tester jobs. What am I missing?

@MartinMystikJonas
Copy link
Contributor

As solution I would suggest never close stderr of job and make RUN_COLLECT_ERRORS either default behaviour or add CLI parameter to turn it on (--collect-errors? --collect-stderr?)

I could prepere PR if you want.

@milo
Copy link
Member

milo commented Jul 8, 2022

The reason was unpredictable behaviour as I wrote earlier there, process hangouts.

@MartinMystikJonas
Copy link
Contributor

MartinMystikJonas commented Jul 8, 2022

@milo Unpredistable behaviour as unpredictalbe order or outputs? Even if stderr is ignored we do not have to close it no? Also when there is both stdout and stderr we can print it something like this:

STDOUT:
1
3
STDERR:
2
4

I have seem this solution used in some tools.

I am not sure what you mean by process hangouts.

@milo
Copy link
Member

milo commented Jul 8, 2022

Sorry, I'm on mobile only... Unpredictable stability. It used to be at least. When stderr was too long, reading from stdout got stuck. Whole process got stuck and has to be killed.

Maybe using 2>&1 is the solution. Just merge stdout and stderr into one.

@MartinMystikJonas
Copy link
Contributor

@milo Actually solution with 2>&1 was my first PR for Nette tester 8 years ago :-) But it was rejected because there were some issues with it too.

See
#86
#89

@MartinMystikJonas
Copy link
Contributor

@milo Is this bug still an issue? I reread discussion in #86 nad PHP bug that caused it https://bugs.php.net/bug.php?id=64438 might be fixed since 2014-09-29

@milo
Copy link
Member

milo commented Jul 9, 2022

Well, it looks like we can try it.

@MartinMystikJonas
Copy link
Contributor

Ok any idea how we can test it? Just run Job that outputs huge stdout and stderr?

@dg
Copy link
Member

dg commented Jul 10, 2022

I tried, it doesn't work even in PHP 8.1.

@MartinMystikJonas
Copy link
Contributor

MartinMystikJonas commented Jul 10, 2022

Damn. Can you sum up how you tested it? Are all OS affected of is it OS specific?

@MartinMystikJonas
Copy link
Contributor

What about that old idea of redirecting stderr to temp file?

@milo
Copy link
Member

milo commented Jul 10, 2022

Since we introduced temporary dir, it is possible. But how to show it/test it? Print it whole when test finished? Or test it separated by new annotation? Or redirect stdout and stderr into the same file and print it, so emulate behaviour of standalone test?

I have no use case for it so I don't know.

@MartinMystikJonas
Copy link
Contributor

MartinMystikJonas commented Jul 10, 2022

IMHO best option would be reditect both stdout and stderr to same file. But last time we discussed it @db did not liked this idea and wanted separated captures.

So I would go for separate capture and keep current behaviour already used with RUN_COLLECT_ERRORS.

And I would output it same way as we output stdout. Just in case there is stderr i would append something like:

some
std
output

--- STDERR ---
some
error
output

I can draft PR and then we can finalize it.

@dg
Copy link
Member

dg commented Jul 10, 2022

Damn. Can you sum up how you tested it? Are all OS affected of is it OS specific?

test.phpt:

<?php file_put_contents('php://stderr', str_repeat('e', 5030));

Job.php line 127:

//		if ($flags & self::RUN_COLLECT_ERRORS) {
			$this->stderr = $stderr;
//		} else {
//			fclose($stderr);
//		}

Tested on Windows 11

@MartinMystikJonas
Copy link
Contributor

I drafted PR #438 with capture of stderr to temp file and added STDERR to test output. Updated tests included.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants