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

Ensure clones of Request are deep clones #218

Merged
merged 5 commits into from Nov 1, 2019
Merged

Ensure clones of Request are deep clones #218

merged 5 commits into from Nov 1, 2019

Conversation

kelunik
Copy link
Member

@kelunik kelunik commented Oct 27, 2019

All properties are either immutable for deep clones now.

Fixes #216.

@kelunik kelunik requested a review from trowski October 27, 2019 20:17
src/Request.php Outdated Show resolved Hide resolved
@trowski
Copy link
Member

trowski commented Oct 28, 2019

The requirement that attributes be serializable makes me rather uncomfortable. This would be a no-go in http-server, so I'm not sure I want to have that kind of restriction in http-client.

@kelunik
Copy link
Member Author

kelunik commented Oct 28, 2019

Our HTTP server doesn't have any retry mechanism. I thought about it pretty much a whole day and this seems to be the only viable solution to do retries to me.

@trowski
Copy link
Member

trowski commented Oct 28, 2019

Do we want to revert Request to be immutable? Being mutable seems to be more trouble than it's worth in the client.

@kelunik
Copy link
Member Author

kelunik commented Oct 28, 2019

I thought about that as well, but it doesn't solve the problem with attributes, because we can't ensure they're immutable. We'd have to deeply clone them like in this solution.

This solution has the added benefit that request and response APIs are aligned without response being not really immutable (if we'd change that back, too).

It's basically the best of both worlds, with the only drawback being a slight performance penalty for attributes, but those shouldn't be used too often.

@trowski
Copy link
Member

trowski commented Oct 28, 2019

Yes – attributes were the reason I didn't like the request being immutable since they weren't really immutable. With this solution, the request could truly be immutable again.

Request headers are still mutable, which I thought was your original concern – DecompressResponse sets the accept-encoding header, but then on a request retry assumes it did not set the header and does not decode the response.

@kelunik
Copy link
Member Author

kelunik commented Oct 28, 2019

They're mutable, yes, but as the request object is required to be cloned at every layer, mutability isn't a problem anymore.

@trowski
Copy link
Member

trowski commented Oct 28, 2019

Requiring cloning is easy to enforce in our code, but I'd be surprised if user interceptors would always remember to clone the request.

@kelunik
Copy link
Member Author

kelunik commented Oct 28, 2019

That's the exact reason why HttpClient instances are required to clone, not interceptors, I've committed that to master already (by accident).

@trowski
Copy link
Member

trowski commented Oct 28, 2019

Ok, enforcing that in the client is a much lower bar that I'm more comfortable with. Looks like we need to add clone in the Stream::request() implementations as well. In turn, we can probably remove clone from the Connection::request() implementations.

kelunik and others added 3 commits October 30, 2019 23:08
All properties are either immutable for deep clones now.

Fixes #216.
We can consider re-adding it in the future.
This allows the connection to modify the request, e.g. add timing information.
@kelunik kelunik merged commit 42dd414 into master Nov 1, 2019
@kelunik kelunik deleted the issue-216 branch November 1, 2019 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Forbid request cloning
2 participants