-
-
Notifications
You must be signed in to change notification settings - Fork 63
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
base: 2.26.x
Are you sure you want to change the base?
Conversation
81de28d
to
7f90f9e
Compare
There was a problem hiding this 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 🤔
There was a problem hiding this 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?
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 <duesterhus@woltlab.com>
7f90f9e
to
e3db859
Compare
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. |
Yes, this is a potential BC break, but cloning a stream is really asking for trouble. |
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 This also raises question about cloning request/response objects. Stream is mutable and it will be shared between the two. |
Very good point made by @Xerkus: the removal of Is it possible to implement safe cloning, until we disallow it in FIG itself? 🤔 |
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. |
This is why this needs to be addressed at FIG level since whatever approach is taken it will defy one expectation or another. |
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.