amphp / http-client

An advanced async HTTP client library for PHP, enabling efficient, non-blocking, and concurrent requests and responses.
https://amphp.org/http-client
MIT License
707 stars 66 forks source link

Forbid request cloning #216

Closed kelunik closed 5 years ago

kelunik commented 5 years ago

Currently Request is still allowed to be cloned, which is used e.g. in FollowRedirects, but this will copy the attributes array and its values and share them, resulting in non-sense if things like response timings are stored in the request attributes (see #193).

We could just empty the attributes on cloning, but then we have to remove all other clone operations in the interceptor chain, because those must not null the attributes, otherwise attributes become useless.

I think the best way forward might be to just entirely forbid cloning. Maybe we should introduce an additional property on request that allows to freeze a request object to prevent further modification?

kelunik commented 5 years ago

I thought about adding Request::createSubRequest()

public function createSubRequest($uri, string $method = "GET"): self
{
    $subRequest = clone $this;
    $subRequest->setUri($uri);
    $subRequest->setMethod($method);
    $subRequest->attributes = [];

    return $subRequest;
}

However, now that I've looked through the usages where we currently clone, it makes me want to restore immutability for requests again, because e.g. the retry interceptor might do weird things, because interceptors might set headers in the first attempt, but then do other things on the second run, because some header is already set now on the request object, e.g. the DecompressResponse interceptor will see that the accept-encoding is already set, so it won't change it and won't decode the response, because it thinks another interceptor does that.

However, we still have a problem with attributes, even if we make request immutable again, because attributes can be mutable. I'm not sure what to do.