Substrate: Updating hyper to 0.12.35 breaks the `api::http::tests::request_write_body_invalid_call` test

[tomaka]

Apparently the behaviour of the sender has changed in latest versions of hyper.
Unfortunately it's not documented in the CHANGELOG, which stopped at 0.12.33.

Pragmatically speaking, we call send_data on the Body, then call poll_ready on that same Body, and it produces a "channel closed" error.

I personally hate hyper and think we should replace it with something else, but the straight-forward solution is to figure out why that happens.

[tomaka]

cc @kianenigma @tomusdrw

[Demi-Marie]

0.12.35 and 0.12.33 are semver-compatible, so this is a bug in Hyper.

[seanmonstar]

I'm sorry you're having this issue! I'm happy to help however!

Apparently the behaviour of the sender has changed in latest versions of hyper.

That's not the intention. When I look at the commits, I don't see anything that touched the Sender at all. But maybe!

Unfortunately it's not documented in the CHANGELOG, which stopped at 0.12.33.

The changelog is still updated in the 0.12.x branch (0.12.33 is just when master started upgrading to std::future). And the releases also contain them.

I'm sorry you hate hyper (and discussing why is off-topic here, though I'd love to know more), but with some details, hopefully we can fix this!

[tomaka]

Thanks for offering to help!

with some details, hopefully we can fix this!

Here some details:

Right now we're using hyper 0.12.33. I've made sure that the sole action of updating to 0.12.35 causes the issue (as in, it works before, I do cargo update -p hyper, and it stops working), so there is probably no external factor.

What the test does (from the point of view of hyper) is:

• We create a Client in a background thread.
• In the main thread, we create a hyper::Body with hyper::Body::channel(), then a hyper::Request around it.
• We send that Request to the background thread. That background thread will then call client.request, then poll the returned Future. When the response arrives, it calls response.into_body() and sends back the result to the main thread.
• Meanwhile, in the main thread, we call poll_ready on the Sender. It returns Ok. Then we call send_data. It returns Ok. Then we call poll_ready again, and it returns Error(ChannelClosed). In hyper 0.12.33, it returned Ok.

The code is here, but it is quite difficult to read: https://github.com/paritytech/substrate/blob/e3f57ff9c86866a040e2c1f5dbe0b994b103e5cd/core/offchain/src/api/http.rs

Note that the test performs queries against a server that is also handled by hyper, so it is possible that a change on the server-side somehow influences the way the client behaves.

I'm sorry you hate hyper (and discussing why is off-topic here, though I'd love to know more),

Sorry for the rudeness of the remark. As someone who is frequently cross-compiling and doing experiments of all kinds, I've had bad experiences with hyper that were related to TLS and tokio, and not to hyper itself.

[seanmonstar]

Ok, I believe I've identified the issue.

There was a bug fix for the client when sending chunked bodies with a GET request: https://github.com/hyperium/hyper/pull/1926. The client has until 0.12.34, forbidden chunked requests, since if a user didn't send anything on a channel (or wrapped stream), we didn't want to send 0\r\n\r\n as the body, as many server didn't understand it. The fix was to allow chunked bodies for GET, but not assume them. If there was a transfer-encoding: chunked header, they could go through, otherwise it'd continue to assume there was no body.

Unfortunately, as part of the fix, this noticed a bug where the body wasn't actually fully ignored (before 0.12.34). It would still be sent, just without any framing (not chunked encoded), but rather like a close-delimited message. However, that's not legal. Without a content-length or transfer-encoding: chunked header, a request body is assumed empty.

I see your test is sending GET requests and using the channel to send data chunks. Before, the body was still sent, but illegally, and now, the body is dropped correctly. To fix it, you'd either need to change to a request method that assumes chunked, like POST, or specifically set a transfer-encoding: chunked header if you plan to send a chunked body on a GET request. Does that make sense?

[Demi-Marie]

@seanmonstar Thank you for your prompt response!

I came to the same conclusion you did, within a minute of you :)

[Demi-Marie]

Switching from GET to POST makes the test pass.