Skip to content

Commit

Permalink
Check writable flag rather than for null resource
Browse files Browse the repository at this point in the history
Also null resource when writing in watcher callback fails. Fixes #20.
  • Loading branch information
trowski committed Sep 15, 2017
1 parent 8047db9 commit fdcf400
Showing 1 changed file with 2 additions and 1 deletion.
3 changes: 2 additions & 1 deletion lib/ResourceOutputStream.php
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ public function __construct($stream, int $chunkSize = null) {
return;
}

$resource = null;
$writable = false;

$message = "Failed to write to socket";
Expand Down Expand Up @@ -144,7 +145,7 @@ public function end(string $finalData = ""): Promise {
}

private function send(string $data, bool $end = false): Promise {
if ($this->resource === null) {
if (!$this->writable) {
return new Failure(new StreamException("The stream is not writable"));
}

Expand Down

4 comments on commit fdcf400

@bwoebi
Copy link
Member

@bwoebi bwoebi commented on fdcf400 Sep 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Umm, I actually suggested directly throwing an Error in #20, in case the stream was ended by ourselves.

@trowski
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's possible, but as far as I'm aware none of the other streams differentiate between the user closing and closing due to an error - both cases result in a StreamException when writing. If doing so wouldn't be considered a BC break (it shouldn't be IMO), then maybe we can do so.

@bwoebi
Copy link
Member

@bwoebi bwoebi commented on fdcf400 Sep 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Converting a condition which must not happen to an \Error is always a bugfix.

@kelunik
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a BC break, because a ClosedException is a StreamException, but directly throwing is a BC break.

Please sign in to comment.