Previously, Agent stored most of its state in one big
Arc<Mutex<AgentState>>. This separates the Arc from the Mutexes.
Now, Agent is a thin wrapper around an Arc<AgentState>. The individual
components that need locking, ConnectionPool and CookieStore, now are
responsible for their own locking.
There were a couple of reasons for this. Internal components that needed
an Agent were often instead carrying around an Arc<Mutex<AgentState>>.
This felt like the components were too intertwined: those other
components shouldn't have to care quite so much about how Agent is
implemented. Also, this led to compromises of convenience: the Proxy on
Agent wound up stored inside the `Arc<Mutex<AgentState>>` even though it
didn't need locking. It was more convenient that way because that was
what Request and Unit had access too.
The other reason to push things down like this is that it can reduce
lock contention. Mutations to the cookie store don't need to lock the
connection pool, and vice versa. This was a secondary concern, since I
haven't actually profiled these things and found them to be a problem,
but it's a happy result of the refactoring.
Now all the components outside of Agent take an Agent instead of
AgentState.
In the process I removed `Agent.cookie()`. Its API was hard to use
correctly, since it didn't distinguish between cookies on different
hosts. And it would have required updates as part of this refactoring.
I'm open to reinstating some similar functionality with a refreshed API.
I kept `Agent.set_cookie`, but updated its method signature to take a
URL as well as a cookie.
Many of ConnectionPool's methods went from `&mut self` to `&self`,
because ConnectionPool is now using interior mutability.
This is a step towards allowing our tests to run without network access,
which will make them more resilient and faster.
Replace the URL in one instance of an HTTPS test that didn't need HTTPS.
In the process, rename set_foo methods to just foo, since methods on the
builder will always be setters.
Adds a new() method on ConnectionPool so it can be constructed directly
with the desired limits. Removes the setter methods on ConnectionPool
for those limits. This means that connection limits can only be set when
an Agent is built.
There were two tests that verify Send and Sync implementations, one for
Agent and one for Request. This PR moves the Request test to request.rs,
and changes both tests to more directly verify the traits. There may be
another way to do this, I'm not sure.
This adds validation of header values on receive, and of both header
names and header values on send. This doesn't change the return
type of set to be a Result, it just validates when the request is
sent. Also removes the section in the README describing handling
of invalid headers, and updates a test that verified acceptance of
non-ASCII headers so that it verifies rejection of them instead.
Gets rid of synthetic_error, and makes the various send_* methods return `Result<Response, Error>`.
Introduces a new error type "HTTP", which represents an error due to status codes 4xx or 5xx.
The HTTP error type contains a boxed Response, so users can read the actual response if they want.
Adds an `error_for_status` setting to disable the functionality of treating 4xx and 5xx as errors.
Adds .unwrap() to a lot of tests.
Fixes#128.
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.
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.
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.
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
Some helper functions introduced as part of the cookies_on_redirect
test are only used by that test, which means they are only used when the
cookie feature is enabled. This causes clippy warnings for some of the
other feature combinations in CI.
Previously this test relied on a specific ordering of cookies, which was
not guaranteed. This makes the test robust to random ordering.
Also, fix the check for the `second` cookie to actually check the right
cookie.
* Put the crate version in the User-Agent header.
https://tools.ietf.org/html/rfc7231#section-5.5.3
Each product identifier consists of a name and optional version.
product = token ["/" product-version]
product-version = token
* User-Agent of rust-ureq
* Update src/unit.rs
Co-authored-by: Martin Algesten <martin@algesten.se>
This doesn't change the API at all, since it delays any errors until do_call.
Since there was already an `Into<Response> for Error`, and do_call already
had a `.unwrap_or_else(|e| e.into())`, nothing in do_call needed to change.
The only visible effect of this is that code that was previously relying on
`get("/path")` to fetch something from localhost will now get an error. I think
most of those cases would probably actually be accidents, possibly caused
by typos, e.g. `get("example.com/path")` (forgetting the https: prefix).
Fixes#105
The layout of crates.io means that code blocks overflow after 60
characters, so wrap before 60 to avoid needing a scrollbar.
Remove the warning about the Agent code. I think after our recent
testing, I believe it's good enough that it doesn't need a separate
warning.
Remove the old-style macro-use directive and the extern crate directive.
The reference to time::Instant under feature = socks-proxy was
incorrectly scoped, and should have been just Instant. This breaks the
doc build and any builds that use feature = socks-proxy.
time_until_deadline had a time of check to time of use problem - the
deadline could pass between a call to checked_duration_since and the
evaluation of `deadline - now` (which panics if the result would be
negative). Resolve that by flipping the order of
checked_duration_since's arguments and using the result rather than
ignoring it.
Also there were three places that called deadline - now(), which could
panic. Replace those with time_until_deadline().
Adds limit of 100 connections to ConnectionPool. This is implemented
with a VecDeque acting as an LRU. When the pool becomes full, the oldest
stream is popped off the back from the VecDeque and also removed from
the map of PoolKeys to Streams.
Fixes#77
This loads a list of top domain names (e.g. from
https://tranco-list.eu/) and tries to fetch them all, in parallel. This
can be used to exercise ureq and find panics.
PoolKey calls unwrap() on an option that can be None. Specifically, the
local variable `port` can be None when PoolKey is constructed with a Url
whose scheme is unrecognized in url.port_or_known_default().
To fix that, make port an Option. Also, make scheme part of the PoolKey.
This prevents, for instance, a stream opened for `https://example.com:9999`
being reused on a request for `http://example.com:9999`.
Remove the test-only pool.get() accessor. This was used in only one test,
agent_pool in range.rs. This seemed like it was testing the agent more
than it was testing ranges, so I moved it to agent.rs and edited to
remove the range-testing parts.
Also, reject unrecognized URLs earlier in connect_socket so they don't
reach try_get_connection.
This also reverts a change to send_body that was originally added to
return the number of bytes written. It's no longer needed now that we
check the size of the reader in advance.
Fixes#76.
In https://github.com/algesten/ureq/pull/67#issuecomment-647112883,
Shnatsel points out that there are uses for read_timeout vs timeout. For
instance, when downloading a large file it's useful to have a read
timouet in case the download stalls, but it would be hard to pick a good
overall timeout without knowing the size of the file and the bandwidth
of the connection.
* Update documentation.
Synchronize goals section between README and rustdoc, and add two goals
(blocking API; no unsafe) that were mentioned elsewhere in the README.
Add error handling to examples for the module and for the Response
object.
And a section on synthetic errors to the top-level module documentation.
* Add back missing close brace.
* Add main function that returns Result.
* Add links to send_bytes and send_form.
* Document chunked encoding for send.
* Use a larger vec of bytes for send.
Adds some feature guards, and removes an unnecessary feature guard
around a call to connect_https (there's an implementation available for
non-TLS that returns UnknownScheme).
Also, remove unnecessary agent.state() method that was only available in
TLS builds. The state field is directly accessible within the crate, and
can be used in both TLS and non-TLS builds.
Co-authored-by: Martin Algesten <martin@algesten.se>
This deprecates timeout_read() and timeout_write() in favor of
timeout(). The new timeout method on Request takes a Duration instead
of a number of milliseconds, and is measured against overall request
time, not per-read time.
Once a request is started, the timeout is turned into a deadline
specific to that call. The deadline is used in conjunction with the
new DeadlineStream class, which sets a timeout on each read according
to the remaining time for the request. Once the request is done,
the DeadlineStream is unwrapped via .into::<Stream>() to become
an undecorated Stream again for return to the pool. Timeouts on the
stream are unset at this point.
Still to be done:
Add a setting on Agent for default timeout.
Change header-writing code to apply overall deadline rather than
per-write timeout.
Fixes#28.
This test was making requests to a server on the Internet. That has the
potential to make the test flaky. Also, the test was relying on a
specific behavior from that server (timing out after 2s), which it no
longer exhibits.
This sets up a local test server that exhibits the specific properties
needed for this test.
This builds on 753d61b. Before we send a request, we can do a 1-byte
nonblocking peek on the connection. If the server has closed the
connection, this will give us an EOF, and we can take the connection out
of the pool before sending any request on it. This will reduce the
likelihood that we send a non-retryable POST on an already-closed
connection.
The server could still potentially close the connection between when we
make this check and when we finish sending the request, but this should
handle the majority of cases.
