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

Use constructor parameters instead of public buffer property #91

Merged
merged 2 commits into from Apr 28, 2017

Conversation

Projects
None yet
3 participants
@clue
Member

clue commented Apr 27, 2017

Remove public $bufferSize property from DuplexResourceStream and ReadableResourceStream and public $softLimit property from WritableResourceStream. These properties have attracted some low quality code and interacting with it in the wrong way could easily break the streams.

Empirical evidence suggests this isn't really needed anyway, so I propose to remove it from the public API for now. Should we need this again in the future, it's a simple feature addition with no BC break, so I'd suggest removing this until we find a use case that actually relies on this.

Refs #90

@clue clue added the BC break label Apr 27, 2017

@clue clue added this to the v0.7.0 milestone Apr 27, 2017

@clue clue force-pushed the clue-labs:buffers branch from 999e6db to d69ad7b Apr 27, 2017

@clue

This comment has been minimized.

Member

clue commented Apr 27, 2017

Rebased to resolve merge conflicts now that #90 is in :shipit:

@WyriHaximus WyriHaximus requested review from jsor and WyriHaximus Apr 27, 2017

@jsor

This comment has been minimized.

Member

jsor commented Apr 27, 2017

Just an idea: what about introducing a withBufferSize() method which returns a new instance.

public function withBufferSize($bufferSize)
{
    $new = new self($this->stream, $this->loop);
    $new->bufferSize = $bufferSize;

    return $new;
}

This way we keep the constructor signature clean and also allow more options without breaking BC in the future.

@clue

This comment has been minimized.

Member

clue commented Apr 28, 2017

@jsor This may or may not be a good API if we need to change the buffer size – however, currently we have yet to come up with a use case that requires changing buffer sizes in the first place.

Afaict this is only really used in react/socket to work around buffering issues on legacy PHP versions, but I've already prepared a PR to simply pass this during construction so we don't need no accessible API anyway.

Arguably, setting buffer sizes in react/socket may even be considered a bad thing, as this means that one may still end up with buffering issues if directly using the stream API. I've explicitly decided to not address this as part of this PR, but the exact wording in the documentation ensures that we may cover this in a future version.

FWIW, Node also only supports setting this during construction: https://nodejs.org/api/stream.html#stream_buffering

My vote here would be to move forward with this proposed change until we run into any issues with more options (which I don't see happen any time soon) and then refactor this, possibly by creating new classes and deprecating the old ones until we reach the next major version :shipit: 👍

@jsor

This comment has been minimized.

Member

jsor commented Apr 28, 2017

I'm not sure i get you're reasoning. If it is discouraged to change the buffer size, why is it even documented and a constructor argument? That's probably one more reaons to have a distinct method for it marked as @internal.

@clue

This comment has been minimized.

Member

clue commented Apr 28, 2017

Changing the buffer sizes is not really "discouraged" as there are some valid use case for this (often used to align with block sizes for disk or network access). But I don't see a single use case that requires changing this during runtime, so my vote goes towards not offering any accessors except during construction.

Note that not all stream implementations use a buffer in the first place. This is in fact only used for our "stream resource" classes and not otherwise available on any other stream implementations such as our own higher level stream logic or the socket component.

This leaves us in a situation where if makes little sense to define this as a public method on any of our interfaces. This in means that if we were to provide accessors (public property or mutable or immutable accessors) then this would require consumers to rely on a concretion, rather than our abstractions.

Besides, changing the buffer sizes during runtime may affect other consumers of the same stream, so I'd vote to not even attempt to address this.

// invalid, because this is not defined on the interface
function do(DuplexStreamInterface $stream) {
    $stream->setBufferSize(1000);
}

// valid, but relies on concretion instead of abstraction
function do(DuplexResourceStream $stream) {
    $stream->setBufferSize(1000);
}
// pretend we need unlimited buffers due to TLS buffering
// with valid type hints
function create(): DuplexStreamInterface {
    $stream = new DuplexResourceStream(…);
    $stream->setBufferSize(null);
}

// let's process individual bytes from this stream
// type hint mismatch as seen above
// naive, but otherwise valid approach
// should work fine with "normal" streams 
// however, breaks TLS buffering again
function process(DuplexStreamInterface $stream) {
    $stream->setBufferSize(1);
    $stream->on('data', function ($byte) { … });
}

$stream = create();
process($stream);
@jsor

This comment has been minimized.

Member

jsor commented Apr 28, 2017

That's not exactly what i suggested (simple setter vs. returning a new instance without listeners). Anyway, it's been just a brain dump. Current solution is ok for me :shipit:

@jsor

jsor approved these changes Apr 28, 2017

@WyriHaximus WyriHaximus merged commit 1284eba into reactphp:master Apr 28, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@clue clue deleted the clue-labs:buffers branch Apr 28, 2017

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