Skip to content
This repository has been archived by the owner on Oct 14, 2022. It is now read-only.

Commit

Permalink
Wait a bit before reading the next socket chunk
Browse files Browse the repository at this point in the history
  • Loading branch information
nesk committed Aug 25, 2018
1 parent aa58944 commit 2dedb65
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 0 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.

## [Unreleased]
### Fixed
- Heavy socket payloads are no longer unreadable in slow environments
- Fix an issue where the console object can't be set in some environments

## [1.2.0] - 2018-08-20
Expand Down
13 changes: 13 additions & 0 deletions src/ProcessSupervisor.php
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,14 @@ class ProcessSupervisor
*/
protected const SOCKET_HEADER_SIZE = 5;

/**
* A short period to wait before reading the next chunk (in milliseconds), this avoids the next chunk to be read as
* an empty string when PuPHPeteer is running on a slow environment.
*
* @var int
*/
protected const SOCKET_NEXT_CHUNK_DELAY = 1;

/**
* Options to remove before sending them for the process.
*
Expand Down Expand Up @@ -404,6 +412,11 @@ protected function readNextProcessValue(bool $valueShouldBeLogged = true)
$chunk = substr($packet, static::SOCKET_HEADER_SIZE);

$payload .= $chunk;

if ($chunksLeft > 0) {
// The next chunk might be an empty string if don't wait a short period on slow environments.
usleep(self::SOCKET_NEXT_CHUNK_DELAY * 1000);
}
} while ($chunksLeft > 0);
} catch (SocketException $exception) {
$this->waitForProcessTermination();
Expand Down

4 comments on commit 2dedb65

@jonnywilliamson
Copy link

Choose a reason for hiding this comment

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

Hi,

Just a few thoughts about this commit. My first thought is WHY does the system need a few millisecs before it can process the next inbound packet?

What is happening in the loop that doesn't allow the decoding/processing of it successfully? Is node just sending things too quickly....but if that were the case, then why would slowing down the PHP processing code help?

Whilst its great that using usleep(1000) for both your system and my system (in homestead anyway) fixes this issue, perhaps the next person to come along might have an ever slower system. Maybe they'll need usleep(5000) etc. It just seems risky to use an arbitrary value and then hope that the problem is fixed.

I'll try and have a look into the code as well to see if I can suggest anything too.

My next thought was the loop in the readNextProcessValue() method. I see that we take each inbound packet, look to see how many more chunks belong to this data block, then remove the header and concatenate the string to a $payload variable.

My concern here would be that if we received a very large data payload from node that we could possibly run out of memory when php is dealing with it. It feels like if the inbound data was held in a stream it would be much safer to deal with large payloads.

A rough outline:

$stream = fopen('php://memory','r+');
fwrite($stream, $chunk);

//After last chunk
rewind($stream);

//Then either return the stream or the contents when needed later:
return stream_get_contents($stream);

Thanks for your help in digging into this.

@nesk
Copy link
Member Author

@nesk nesk commented on 2dedb65 Aug 26, 2018

Choose a reason for hiding this comment

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

You are totally right! I was thinking about using streams yesterday, they will be necessary if a heavy screenshot is transferred (or another type of data). However, if you're calling the screenshot method with PuPHPeteer, you're expecting it to return a string (as specified in Puppeteer's documentation), not a resource, so if we need to transform the resource to a string, there is no benefit. Maybe we could add an option to return strings as streams? But I'm not sure about this.

Honestly, the socket part of my code is really crappy (especially in PHP), I don't have that much experience with them. I can't even remember why I'm splitting the payloads in chunks of 1024 bytes…

A refactoring will be needed, I'm totally for it. 👍

@nesk
Copy link
Member Author

@nesk nesk commented on 2dedb65 Aug 31, 2018

Choose a reason for hiding this comment

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

@jonnywilliamson You might be interested in following issue #12.

(and thank you for the beer! 🍺)

@jonnywilliamson
Copy link

Choose a reason for hiding this comment

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

Yes I am!

Let me have a look and see if I can help with anything.

Please sign in to comment.