Skip to content
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

Disallow cloning Stream objects #141

Draft
wants to merge 1 commit into
base: 2.26.x
Choose a base branch
from

Conversation

TimWolla
Copy link
Contributor

Q A
Documentation no
Bugfix ?
BC Break ?
New Feature no
RFC yes
QA no

Description

Not necessarily intended to be merged, primarily intended to put this up for discussion with an actual code change instead of just raising an issue. I did not encounter this in a real-world app, this was more of a “what if”.


Streams hold a reference to the stateful resource handle for their actual contents. Cloning a Stream will not actually clone the underlying resource, thus both streams would still refer to the same resource after cloning and any changes in one stream object would be reflected in the other object. This violates user expectations after a cloning operation.

Disallow cloning entirely as the safe default choice. Alternatively a new stream could be created and attached and the contents could be copied over. This can get expensive with larger or infinite streams, though.

@froschdesign
Copy link
Member

@TimWolla

Not necessarily intended to be merged, primarily intended to put this up for discussion…

Then I convert this pull request into a draft.

@froschdesign froschdesign marked this pull request as draft April 13, 2023 08:56
@TimWolla TimWolla force-pushed the stream-unclonable branch from 81de28d to 7f90f9e Compare April 13, 2023 09:00
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

The proposal is very much sensible: due to streams being a squishy mess for the foreseeable future, it's best to not allow multiple references to the same stream in different objects, and cloning should be disallowed.

The newly introduced test could perhaps even be dropped 🤔

Copy link
Member

@gsteel gsteel left a comment

Choose a reason for hiding this comment

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

LGTM! This would be considered a BC break no?

test/StreamTest.php Outdated Show resolved Hide resolved
Streams hold a reference to the stateful resource handle for their actual
contents. Cloning a Stream will not actually clone the underlying resource,
thus both streams would still refer to the same resource after cloning and any
changes in one stream object would be reflected in the other object. This
violates user expectations after a cloning operation.

Disallow cloning entirely as the safe default choice. Alternatively a new
stream could be created and attached and the contents could be copied over.
This can get expensive with larger or infinite streams, though.

Signed-off-by: Tim Düsterhus <[email protected]>
@TimWolla TimWolla force-pushed the stream-unclonable branch from 7f90f9e to e3db859 Compare April 13, 2023 13:02
@TimWolla
Copy link
Contributor Author

This would be considered a BC break no?

This technically is a compatibility break, yes. That's the primary reason why I've mentioned the “put up for discussion” part. I'll leave it to you to decide whether this is a bugfix or a debugging aid and whether the compatibility is relevant or not.

@Ocramius
Copy link
Member

Yes, this is a potential BC break, but cloning a stream is really asking for trouble.

@Xerkus
Copy link
Member

Xerkus commented Apr 13, 2023

This looks like something that needs to be brought to FIG to explicitly define expected clone behavior.

I see a benefit for well defined clone behavior where stream is expected to do internal stream_copy_to_stream(). May be for original stream object too if resource is not rewindable.

This also raises question about cloning request/response objects. Stream is mutable and it will be shared between the two.

@Ocramius
Copy link
Member

Very good point made by @Xerkus: the removal of clone support on our Stream could easily break other message implementations 🤔

Is it possible to implement safe cloning, until we disallow it in FIG itself? 🤔

@TimWolla
Copy link
Contributor Author

Is it possible to implement safe cloning, until we disallow it in FIG itself? thinking

Depends: Infinite streams are unsafe, but one could tell users “don't do that”. I'm also not sure about unrewindable generator-based streams.

Anyway: If your concerned about interoperability then even “safe cloning” is unsafe, because an implementation might actually expect the underlying stream resource to remain the same after a clone. This is an … odd assumption, but still an assumption that might've been made.

@Xerkus
Copy link
Member

Xerkus commented May 1, 2023

This is an … odd assumption, but still an assumption that might've been made.

This is why this needs to be addressed at FIG level since whatever approach is taken it will defy one expectation or another.

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

Successfully merging this pull request may close these issues.

5 participants