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

Bug in onReadable #65

Closed
withinboredom opened this Issue Feb 1, 2017 · 3 comments

Comments

3 participants
@withinboredom
Copy link

withinboredom commented Feb 1, 2017

It looks like this function doesn't exist in 2.0?

However, this affects Aerys with php-uv reactor.

In native:

Amp\onReadable( $this->socket, function ( $watcherId, $socket ) use ( &$data ) {
  $data[2] .= @fread( $socket, 1024 );
  . . . 

That will work, even if the fread doesn't read all the data in one go, it will still call the function on the next tick. With php-uv, it will not. It appears that it has to wait until new data hits the socket. This means that a programmer would have to know the maximum buffers of the socket in the target environment, or risk missing the tail end of a transaction.

@bwoebi bwoebi added the documentation label Feb 1, 2017

@bwoebi

This comment has been minimized.

Copy link
Member

bwoebi commented Feb 1, 2017

The reason here is that the default chunk size of PHP is 8192. If you read less than that, PHP may have locally buffered data, which just stream_select() is aware of. (Other backends like libevent aren't aware of it either.)
If you use any multiple of 8192 in fread, you won't occur such an issue.
Or you have to explicitly lower the stream chunk size for that stream via stream_set_chunk_size().

It's not per se a bug, but one does have to be aware of that behavior and needs to be documented accordingly.

p.s.: In Amp v2.0 we currently recommend using \AsyncInterop\Loop::onReadable() directly. (but that may change with async-interop/event-loop#149)

@kelunik

This comment has been minimized.

Copy link
Member

kelunik commented Feb 9, 2017

@bwoebi Could you add a PR to async-interop/event-loop regarding that?

kelunik pushed a commit that referenced this issue Mar 10, 2017

Merge pull request #65 from async-interop/lazy-fast-loop
Lazy factory instantiation and save method call
@kelunik

This comment has been minimized.

Copy link
Member

kelunik commented May 3, 2017

@bwoebi Should probably be stream_set_read_buffer() instead of stream_set_chunk_size()? Or does stream_set_chunk_size() set the size for read and write?

@kelunik kelunik closed this in 3bb21b8 Nov 11, 2017

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