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

ResourceOutputStream treats open stream as closed #58

Closed
psafarov opened this issue May 1, 2019 · 5 comments · Fixed by #60
Closed

ResourceOutputStream treats open stream as closed #58

psafarov opened this issue May 1, 2019 · 5 comments · Fixed by #60
Labels

Comments

@psafarov
Copy link

psafarov commented May 1, 2019

Here is a problem which occurs if the script is run from systemd

Amp\Loop::run(function() {
    $resourceOutputStream = new ByteStream\ResourceOutputStream(\STDOUT);
    // Throws Amp\ByteStream\StreamException: The stream was closed by the peer
    yield $resourceOutputStream->write('test');
});

I figured out that it stops happening if feof check of the stream is removed in ResourceOutputStream.php:74. Apparently it is not correct to use feof for checking if stream is open or not, but I am not sure what can be done here as it has been added for a reason.

@bwoebi
Copy link
Member

bwoebi commented May 6, 2019

No idea why feof() is being used, this only makes sense for readability checks - it has no purpose in ResourceOutputStream.

@trowski can you please revisit #40? The fix looks not correct to me...

@trowski
Copy link
Member

trowski commented May 6, 2019

@bwoebi I think what really fixed #40 was failing after multiple writes of 0 length. On macOS feof() detects IPC pipes being closed, but I always had mixed results with network sockets. If it causes trouble on other systems, we can probably drop the two if-statements with feof() and rely on failing after multiple 0-length writes.

@kelunik
Copy link
Member

kelunik commented May 6, 2019

Failing after multiple 0-length writes still isn't the optimal solution, but might be OK given we don't really support TLS renegotiation.

@kelunik kelunik added the bug label May 8, 2019
@kelunik
Copy link
Member

kelunik commented May 8, 2019

Running the example via the shell yields the following output:

array(9) {
  ["timed_out"]=>
  bool(false)
  ["blocked"]=>
  bool(true)
  ["eof"]=>
  bool(false)
  ["wrapper_type"]=>
  string(3) "PHP"
  ["stream_type"]=>
  string(5) "STDIO"
  ["mode"]=>
  string(2) "wb"
  ["unread_bytes"]=>
  int(0)
  ["seekable"]=>
  bool(true)
  ["uri"]=>
  string(12) "php://stdout"
}
test

Running the same script with systemd yields:

array(9) {
  ["timed_out"]=>
  bool(false)
  ["blocked"]=>
  bool(true)
  ["eof"]=>
  bool(false)
  ["wrapper_type"]=>
  string(3) "PHP"
  ["stream_type"]=>
  string(10) "tcp_socket"
  ["mode"]=>
  string(2) "r+"
  ["unread_bytes"]=>
  int(0)
  ["seekable"]=>
  bool(false)
  ["uri"]=>
  string(12) "php://stdout"
}
PHP Fatal error:  Uncaught Amp\ByteStream\StreamException: The stream was closed by the peer

kelunik added a commit that referenced this issue May 8, 2019
The previous mechanism with feof doesn't work for systemd launched processes.

Fixes #58.
kelunik added a commit that referenced this issue Jun 3, 2019
The previous mechanism with feof doesn't work for systemd launched processes.

Fixes #58.
kelunik added a commit that referenced this issue Jun 3, 2019
The previous mechanism with feof doesn't work for systemd launched processes.

Fixes #58.
@kelunik
Copy link
Member

kelunik commented Jun 3, 2019

Finally released v1.6.0. 🎉

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

Successfully merging a pull request may close this issue.

4 participants