This feature was broken in #67, which reset timeouts on the
stream before passing it to set_stream.
As part of this change, refactor the internal storage of
timeouts on the Request object to use Option<Duration>.
Remove the deadline field on Response. It wasn't used. The
deadline field on unit was used instead.
Add a unittest.
CONNECT, specified at https://tools.ietf.org/html/rfc7231#section-4.3.6,
says:
> if successful, thereafter restrict its behavior to blind forwarding
> of packets, in both directions, until the tunnel is closed.
ureq doesn't actually support the semantics of CONNECT, since it doesn't
offer a bidirectional channel on a Response. So I'm fairly confident no
one is using these methods.
CookieJar doesn't support the path-match and domain-match algorithms from [RFC 6265](https://tools.ietf.org/html/rfc6265#section-5.1.3), while cookie_store does.
This fixes some issues with the cookie matching algorithm currently in ureq. For instance,
the domain-match uses substring matching rather than the RFC 6265 algorithm.
This deletes two tests:
match_cookies_returns_nothing_when_no_cookies didn't test much
agent_cookies was failing because cookie_store rejects cookies on the `test:` scheme.
The way around this is to set up a testserver - but it turns out cookies_on_redirect already
does that, and covers the same cases and more.
This changes some cookie-related behavior:
- Cookies could previously be sent to a wrong domain - e.g. a cookie set on `example.com`
could go to `example.com.evil.com` or `evilexample.com`. Probably no one was relying on
this, since it's quite broken.
- A cookie with a path of `/foo` could be sent on a request to `/foobar`, but now it can't.
- Cookies could previously be set on IP addresses, but now they can't.
- Cookies could previously be set for domains other than the one on the request (or its
parents), but now they can't.
- When a cookie had no domain attribute, it would previously get the domain from the
request, and subsequently be sent to that domain and all subdomains. Now, it will only
be sent to that exact domain (host-only).
That last one is probably the most likely to break people, since someone could depend
on it without realizing it was broken behavior.
This emulates the test matrix that gets run in CI, making it easier to
find failures locally.
There was one conflict in the matrix: when JSON is enabled and TLS is
disabled, two of the doctests would fail. This was previous worked
around as an exclude in the github workflow. I changed the JSON doctest
to use HTTP instead.
When running tests locally, this error can surface.
```
---- test::agent_test::custom_resolver stdout ----
thread 'test::agent_test::custom_resolver' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 22, kind: InvalidInput, message: "Invalid argument" }', src/stream.rs:60:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
```
The problem is that setting the timeouts might fail, and this is done
in a From trait where there is not possibility to "bubble" the
io::Error.
```
socket.set_read_timeout(None).unwrap();
socket.set_write_timeout(None).unwrap();
```
This commit moves the resetting of timers to an explicit `Stream::reset()` fn
that must be called every time we're unwrapping the inner stream.
Previously we had a special case for BadStatusRead that would happen
only when we got a ConnectionAborted error reading the status line.
However, sometimes we get ConnectionReset instead. Also the HTTP
spec says that idempotent requests may be retried anytime a connection
is closed prematurely.
The change treats as retryable any ConnectionAborted OR ConnectionReset
error while reading the status line and headers. It removes the special
case BadStatusRead error.
Fixes#165 (I think).
Previously any io::Error on reading a status line or response headers
would be unconditionally mapping into BadStatus or BadHeader. I think
it's better to pass through the actual io::Error.
BadStatusRead is still kept, since it has special status when dealing
with timed out connections, and BadStatus is still used when the status
line is malformed.
In a few places we relied on "localhost" as a default if a URL's host
was not set, but I think it's better to error out in these cases.
In general, there are a few places in Unit that assumed there is a
host as part of the URL. I've made that explicit by doing a check
at the beginning of `connect()`. I've also tried to plumb through
the semantics of "host is always present" by changing the parameter
types of some of the functions that use the hostname.
I considered a more thorough way to express this with types - for
instance implementing an `HttpUrl` struct that embeds a `Url`, and
exports most of the same methods, but guarantees that host is always
present. However, that was more invasive than this so I did a smaller
change to start.
Previously, using `.send()` on `Request` would require to set either the
Transfer-Encoding or the Content-Length header.
In an effort to provide better ergonomics for this library and to avoid
making users fall in a not-so obvious pitfall, the library should build a
valid Request without asking the user to mess around with the headers.
This commit attempts to fix this issue: if the user use `.send()` to
provide an unknown sized reader, the chunked Transfer-Encoding will be
used. Of course, there are prior checks to ensure we do not override the
user wish, like if the user already set a Content-Length or a
Transfer-Encoding.
This commit fix an edge case where the Content-Length would not be
automatically set if the Transfer-Encoding is set but not chunked.
We only want streams in the pool that are ready for the next request,
and don't have any remaining bytes of the previous request. That's the
case when we return a stream due to EOF on the underlying `Read`.
However, there was also an `impl Drop` that returned the stream to the
pool on drop of a PoolReturnRead. That resulted in a bug: If a user
called `response.into_reader()`, but did not read the entire body,
a dirty stream would be returned to the pool. The next time a request
was made to the same host, that stream would be reused, and instead of
reading off "HTTP/1.1 200 OK", ureq would read a remnant of the previous
request, resulting in a BadStatus error.
This broke during some recent refactorings. The special case branch for
max_connections == 0 wasn't actually setting the max_idle_connections
field.
This change simply deletes that branch, since the existing code that
whittles down the pool size one element at a time suffices. In the
common case this will be set on an empty pool anyhow.
This defines a new trait `Resolver`, which turns an address into a
Vec<SocketAddr>. It also provides an implementation of Resolver for
`Fn(&str)` so it's easy to define simple resolvers with a closure.
Fixes#82
Co-authored-by: Ulrik <ulrikm@spotify.com>
Instead of cloning most of `Request`'s fields individually when
creating a `Unit`, this PR switches to just cloning `Request` and
stuffing it in `Unit`, and changes references to `unit.[field]` to
`unit.req.[field]` where appropriate.
Fixes#155
Now that we allow multiple connections for the same PoolKey, update our
invariants accordingly. Also provide a couple of helper functions for
removing the first or last match of an entry in a VecDeque.
This also changes which entry from `lru` gets removed when a stream is
removed from the pool. Previously it was the oldest matching one. Now it's
the newest matching one, which matches the semantics we are applying to
`recycle[K]`.
Followup to #133
/cc @cfal
