Previously, we treated HTTP/1.0 responses as always being
close-delimited. However, that's not quite right. HTTP/1.0 responses
_default_ to Connection: close, but the server may send Connection:
keep-alive, along with a Content-Length header. Update body_type to
understand this.
Also, slightly reorganize body_type. has_no_body was being checked
redundantly when we could just early-return when has_no_body is true. And
regardless of whether we see Connection: close (on any HTTP version),
we must honor Transfer-Encoding or Content-Length if they are present.
Change the handling of `Connection: close` to more directly affect
whether a connection is pooled, regardless of what method was used
to delimit the body.
Fixes#600
These test were failing because they made live requests to httpbin.org,
which is serving intermittent 500s.
In general we strive to make our test and doctests interact with a local
test server rather than a live website. For doctests we do this with
some sleight of hand in a hidden `ureq::is_test(true)`, which swaps out
the default agent returned by ureq::agent. However, since the middleware
doctests rely on constructing a custom agent with the middleware, they
can't rely on this.
I suspect we can get around this problem but it may take some thinking.
Similarly disable the AgentBuilder::user_agent and redirects doctests
and the agent_set_header unittest.
This breaks a reference cycle between PoolReturner and Agent that was
causing Agents (and their contained ConnectionPool) to never be dropped
so long as there was any stream in the ConnectionPool. This cause
sockets to leak over time, particularly when the convenience functions
ureq::get(), ureq::post(), etc were used, since those functions create
a new Agent each time.
Introduce PoolReturner, a handle on an agent and a PoolKey that is
capable of returning a Stream to a Pool. Make Streams keep track of
their own PoolReturner, instead of having PoolReturnRead keep track of
that information.
For the LimitedRead code path, get rid of PoolReturnRead. Instead,
LimitedRead is responsible for returning its Stream to the Pool after
its second-to-last read. In other words, LimitedRead will return the
stream if the next read is guaranteed to return Ok(0).
Constructing a LimitedRead of size 0 is always wrong, because we could
always just return the stream immediately. Change the size argument to
NonZeroUsize to enforce that.
Remove the Done trait, which was only used for LimitedRead. It was used
to try and make sure we returned the stream to the pool on exact reads,
but was not reliable.
This does not yet move the ChunkDecoder code path away from
PoolReturnRead. That requires a little more work.
Part 1 of #559. Fixes#555.
This allows removing the hack where we create a Response with an empty `reader`,
then immediately mutate it to set the real reader. It also happens to allow us
to get rid of 3 fields of Response that were only used to pass information to
`stream_to_reader`.
I've tried to keep the structure and logic of the body calculation as close to
the same as possible, for ease of review and to avoid introducing bugs. I think
there are some followup fixes we can make to the logic, which will be made
easier by having it in a self contained function.
This is more correct, since all gzip streams can consist of multiple members. It
has the happy side effect that it causes gzipped responses to reliably return
their streams to the pool.
* Response: build reader at construction time
* Remove unwrapping/rewrapping DeadlineStream
* Let into_reader() provide Sync + 'static
* Prebuffer Vec use known capacity
Co-authored-by: Martin Algesten <martin@algesten.se>
Previously, ReadWrite had methods `is_poolable` and `written_bytes`, which
were solely for the use of unittests.
This replaces `written_bytes` and `TestStream` with a `struct Recorder`
that implements `ReadWrite` and allows unittests to access its recorded
bytes via an `Arc<Mutex<Vec<u8>>>`. It eliminates `is_poolable`; it's fine
to pool a Stream of any kind.
The new `Recorder` also has some convenience methods that abstract away
boilerplate code from many of our unittests.
I got rid of `Stream::from_vec` and `Stream::from_vec_poolable` because
they depended on `TestStream`. They've been replaced by `NoopStream` for
the pool.rs tests, and `ReadOnlyStream` for constructing `Response`s from
`&str` and some test cases.
We unwrap the stream exactly once per response, and we know that case
will be uncontended for the same reason `SyncWrapper` works:
`into_reader()` takes `self`, so it must have exclusive ownership.
Uncontended mutexes are extremely cheap. This saves us a dependency
at a trivial performance cost.
This fixes an issue where requests with a deadline would do a syscall
on every read, rather than pulling from the buffer. Fixes a problem
reported in #506 where `resp.into_json()` was unnecessarily slow.
If the user reads exactly the number of bytes in the response, then
drops the Read object, streams will never get returned to the pool since
the user never triggered a read past the end of the LimitedRead.
This fixes that by making PoolReturnRead aware of the level below it, so
it can ask if a stream is "done" without actually doing a read.
Also, a refactorign:
Previously, Response contained an Option<Box<Unit>> because the testing
method `from_str()` would construct a Response with no associated Unit.
However, this increased code complexity with no corresponding test
benefit. Instead, construct a fake Unit in from_str().
Also, instead of taking an `Option<Box<Unit>>`, PoolReturnRead now takes
a URL (to figure out host and port for the PoolKey) and an &Agent where
it will return the stream. This cuts interconnectedness somewhat:
PoolReturnRead doesn't need to know about Unit anymore.
This makes us less likely to try and reuse a closed connection, which
produces problems in particular for requests that can't be retried.
Fixes#361Fixes#124
This prevents a malicious server from running the client out of memory
by returning a very large body.
Limit into_string() to 1 megabyte.
Limit individual header fields to 100 kilobytes.
Limit number of header fields to 100.
Part of #267
This was triggering for errors like InvalidCertificate and giving the
message "Sometimes this means the host doesn't support any of the same
ciphersuites as rustls, or doesn't support TLS 1.2 and above," which is
confusing in that situation.
For now, offer no specialized error. I'd like to come back when I find
more time and restore this, but only for the error cases where it's
useful.
Contrary to smoke-test, this takes full URLs on the commandline and
prints their contents to stdout. This makes it easier to test behavior
with specific URLs. I hope to later add flags for various behaviors like
printing headers, following redirects, enabling / disabling cookies,
and verbose output.
Also add a useful debug line when receiving a cookie header.
Status code must be exactly three digits.
HTTP version must be "HTTP/", digit, dot, digit.
https://tools.ietf.org/html/rfc7230#section-3.1.2
status-line = HTTP-version SP status-code SP reason-phrase CRLF
After the recent changes in #257, it's probably not necessary. It's now
quite easy to use a match statement to extract responses for certain
status codes, or all status codes.
Add documentation on how to turn a status code error back into a
Response.
This allows Error to report both the URL that caused an error, and the
original URL that was requested.
Change unit::connect to use the Response history for tracking number of
redirects, instead of passing the count as a separate parameter.
Incidentally, move handling of the `stream` fully inside `Response`.
Instead of `do_from_read` + `set_stream`, we now have `do_from_stream`,
which takes ownership of the stream and keeps it. We also have
`do_from_request`, which does all of `do_from_stream`, but also sets the
`previous` field.
We should include the `fn main() -> Result` part, because people may not
know they need the `-> Result` in order for `?` to work.
Also, I replaced `Accept: text/html` with `Example-Header: header
value`. My thinking is that if someone doesn't know they need to remove
that Accept for non-HTML URLs, they might get unexpected results.
After the recent changes in #257, it's probably not necessary. It's now
quite easy to use a match statement to extract responses for certain
status codes, or all status codes.
Change "Bad" to "Invalid" in error names, mimicking io::Error::ErrorKind.
Change InvalidProxyCreds to ProxyUnauthorized.
Change DnsFailed to just Dns (the fact that there was a failure is implicit
in the fact that this was an error).
Now that Responses with non-2xx statuses get turned into `Error`,
there is less need for these. Also, surveying the set of public crates
that depend on ureq, none of them use these methods. It seems that
users tend to prefer checking the status code directly.
Here is my thinking on each of these individually:
.ok() -- With the new Result API, any Request you get back will be
.ok(). Also, I think the name .ok() is a little confusing with
Result::ok().
.error() - with the new Result API, this is an exact overlap with
anything that would return Error. People will just check for whether a
Result is Err(...) rather than call .error().
.client_error() - most of the time, if someone wants to specially handle
a 4xx error, they want to handle specific ones, because the response to
them is different. For instance a specialized response to a 404 would be
"delete this from the list of URLs to check in the future," where a
specialized response to a 401 would be "try and load updated
credentials." For instance:
https://github.com/msfjarvis/healthchecks-rs/blob/4200edb9ed73bed56a05d81cdd3b0a65badc7ede/healthchecks/src/manage.rs#L70-L84https://github.com/SirWindfield/cargo-free/blob/75d4b363b63fa2b6f3ac12a196a2cb80f0afc848/src/lib.rs#L59-L63https://github.com/lukehsiao/netlify-ddns-rs/blob/1d7daea38bf78d9f1bbfba9d38ea1863ed70ed1d/src/netlify.rs#L101-L112
.server_error() - I don't have as much objection to this one, since it's
reasonable to want to treat all server errors (500, 502, 503) more or
less the same. Although even at that, 501 Not Implemented seems like
people would want to handle it differently. I guess that doesn't come up
much in practice - I've never seen a 501 in the wild.
.redirect() - Usually redirects are handled under the hood, unless
someone disables automatic redirect handling. I'm not terribly opposed
to this one, but given that no-one's using it and it's just as easy to
do 300..399.contains(resp.status()), I'm mildly inclined towards
deletion.
Also, remove Response::{ok, error, client_error, server_error,
redirect}. The idea is that you would access these through the
Error object instead.
I fetched all the reverse dependencies of ureq on crates.io and looked
for uses of the methods being removed. I found none.
I'm also considering removing the error_on_non_2xx method entirely. If
it's easy to get the underlying response for errors, it would be nice to
make that the single way to do things rather than support two separate
ways of handling HTTP errors.
This moves Stream's enum into an `Inner` enum, and wraps a single
BufReader around the whole thing. This makes it easier to consistently
treat the contents of Stream as being wrapped in a BufReader.
Also, implement BufRead for DeadlineStream. This means when a timeout
is set, we don't have to set that timeout on the socket with every
small read, just when we fill up the buffer. This reduces the number
of syscalls.
Remove the `Cursor` variant from Stream. It was strictly less powerful
than that `Test` variant, so I've replaced the handful of Cursor uses
with `Test`. Because some of those cases weren't test, I removed the
`#[cfg(test)]` param on the `Test` variant.
Now that all inputs to `do_from_read` are `impl BufRead`, add that
as a type constraint. Change `read_next_line` to take advantage of
`BufRead::read_line`, which may be somewhat faster (though I haven't
benchmarked).
