psr7 icon indicating copy to clipboard operation
psr7 copied to clipboard

Inconsistent Stream state in factory methods (?)

Open mindplay-dk opened this issue 5 years ago • 4 comments

This issue may be a bit controversial, because the spec (PSR-17) appears to be incomplete on this point.

The issue is with the state of the internal stream produced by createStreamFromFile() and createStream() respectively.

While createStreamFromFile() produces a stream that points to the start of the file, createStream() produces a stream that points to the end of the file.

Producing a stream that points to the end of the file is problematic, since the most likely use of these streams is in the body of a Response - and you end up producing an empty response.

It's arguably a bit inconsistent and surprising, as these are just two different ways to create a stream from some pre-existing content - I think it's reasonable to expect them to work the same?

Diactoros is a popular source of reference for this, and it does rewind the stream when it creates a stream from a string.

Creating a stream from a string with the intent of writing to it is a much less likely scenario - if I was doing that, I'd probably start with createStream("") anyhow, and it would work either way, since the file pointer would be at 0 either way.

This has already lead to some pretty serious bugs while switching from Diactoros to this package.

Can we change it please?

Let me know if you'd accept a PR.

mindplay-dk avatar Jan 07 '19 08:01 mindplay-dk

Looks like there's already an ongoing discussion about this issue here.

mindplay-dk avatar Jan 07 '19 08:01 mindplay-dk

Let me know if you'd accept a PR.

For now: no. (Unless @Nyholm over-rules me.) There was a PR for this already, #98, but it was closed by its author after I blocked it pending further discussion. That said:

I am super aware of this issue, as I am the person who also raised it (and did the multi-implementation testing) over on the mailing list. This will definitely be fixed in some way, but before diving in head first I would like to give the PHP-FIG WG a chance to discuss. This will hopefully ensure we do not end up implementing something today that needs to be redone tomorrow.

For now, if you need the ability to switch between PSR-7 implementations, you have two options:

  1. Cast the object to a string, __toString() is specified by PHP-FIG to return as much content as possible (from start to end, if seeking to the beginning is possible on the current resource).
  2. Always use some combination of tell()/seek()/rewind() if you depend on using read().

Zegnat avatar Jan 10 '19 10:01 Zegnat

I agree with @Zegnat.

I see no reason to change unless a PSR is updated with clear guidelines.

And as I wrote on the mailing list, I dont think a PSR should specify this.

Nyholm avatar Jan 10 '19 10:01 Nyholm

Have you both read my proposed rationale for this change?

Do you understand why neither of the two options you're proposing are viable?

Specifically:

  1. "the __toString() method results in unacceptable memory overhead for large streams"
  2. "An emitter can make no assumptions about streams (or their internal resource handles) being repeatable, and therefore can't simply rewind() the stream."

Not all streams are seekable in the first place, so (2) isn't even always possible.

We have real use-cases (such as controllers that pass thru a stream from an S3 service) and applying any of your proposed work-arounds at the emitter-level would be detrimental to performance and reliability.

We also plan on running Swoole (or some other form of PHP-level daemon) in production - while __toString() is often used in CGI emitters, it can't be used in stream-based daemonized emitters.

You might think we could address this individually in each controller - but even that is not an option when you're using something like FlySystem, since you don't know where a given stream-resource comes from, e.g. could be bootstrapped with an S3, HTTP or local file-system, you simply can't know.

mindplay-dk avatar Jan 13 '19 11:01 mindplay-dk

Per my earlier notes:

While createStreamFromFile() produces a stream that points to the start of the file, createStream() produces a stream that points to the end of the file.

This is still an issue - for example, trying to arrange a stream for returning or testing:

$body = (new Psr17Factory)->createStream(json_encode(["foo" => 123]));

Without first rewinding the body, you will get nonsensical exceptions from various facilities, often things like "the body isn't valid JSON", etc. - consumers, as explained, can't just rewind the stream, so it's up to the producer of a stream to get it into the right state.

Years later, I still don't understand why createStream() doesn't rewind the stream, if the content is given?

  • If you're creating an empty stream, you're most likely going to write additional content to it - that would move the stream pointer, and then of course you would expect the stream pointer to end up at the end of the stream.

  • If you're creating a stream with content given, you're most likely not going to write more content to it afterwards. To avoid confusion, I'd suggest this should even be a read-only stream.

At least, this would give you a meaningful error message you can act on.

mindplay-dk avatar Dec 11 '22 16:12 mindplay-dk