Regression 0.11.12 -> 0.11.13: hangs when processing body in `Service`

[sanmai-NL]

A Service with the following endpoint in its call method:

(Post, "/endpoint") => {
    debug!("Hang?");
    let fut = body
        .concat2()
        .and_then(move |body: hyper::Chunk| {
            debug!("No");
            Ok(())
        })
        .and_then(|_| {
            Ok(Response::new().with_status(StatusCode::Ok))
        });
    Box::new(fut)
}

Hangs under 0.11.13, but not under 0.11.12. As tested with:

curl -k -XPOST 'https://localhost:4443/endpoint' -d '[[]]'

[FalacerSelene]

I'm also being affected by this - for now I've pinned to version 0.11.12.

[seanmonstar]

Hm, there must be something more to this, because making a server with the above code and running curl against has it working for me.

[seanmonstar]

Ok, I found the missing piece: using hyper with TcpServer. Looking into the cause.

[sanmai-NL]

@seanmonstar: not meaning to press you on this at all, but very curious how in retrospect you feel about making a substantial change to Hyper in a patch release, the coming about of this bug, your decision to fix it with a new patch release without yanking the affected version? Is there something about this chain of events that you take as a lesson?

Are you planning to yank 0.11.13?

[seanmonstar]

how in retrospect you feel about making a substantial change to Hyper in a patch release?

I still feel pretty good about it, as it has 1) remove many less obvious bugs that existed because of the tokio dispatcher, and 2) has moved hyper forward significantly for when the tokio reform is complete.

As hyper is currently pre-1.0, semver is basically shifted over one place. So, going from 0.11.11 to 0.11.12 is fine with adding new features (in this case, new APIs), yet most of the work was more of a refactor to not depend on a internal dependency. If hyper were already 1.0, I'd probably consider the work something to go into a minor version, but it's not yet!

the coming about of this bug

Software has bugs. It's unfortunate. I hate it. But it's a fact.

This one was reported over the weekend, but getting after getting back to work, it was fixed the same day I had time to look at it.

This particular bug was not introduced in the large refactor, which was release in 0.11.11, but rather in a later bug fix that was released in 0.11.13. The reason for this bug being released is how most bugs get released: there wasn't test coverage for this specific instance. As part of the bug fix, the commit also included test coverage that triggers the bug if the fix is not in place.

without yanking the affected version? Are you planning to yank 0.11.13?

I don't believe so, no. Yanking isn't meant to be used on every single version that happens to contain bugs. Yanking is to remove a version with a serious issue, perhaps legal or security related. Any user would get same behavior from cargo update whether I yanked 0.11.13 or not.

Is there something about this chain of events that you take as a lesson?

Not especially. I'm terribly sorry that a bug was introduced, I hate that I write bugs. I'm sorry if it caused issues in your deployment. But for this particular chain of events, I feel like it was handled promptly. The only thing I could think of myself is that it'd be great to have had the test before, but that's the problem with tests: we only ever have tests of the bugs we think of, not of those we don't.

[sanmai-NL]

@seanmonstar: thanks for sharing your thoughts. Again, not trying to press you. You do great work and I haven’t paid for it. Also, be clear there’s no question in my mind you addressed the bug well!

I personally felt the bug is serious, because it hangs the server and doesn’t even trigger a crash. So it would be possible that someone deploys some service with an updated (patch release) Hyper dep and then suddenly suffer from unavailability of all POST endpoints. Basically it’s also a DoS vuln.
Those are my 2 cents.