Fix HTTPS request deadlock

[mb64]

Fixes #144.

HTTPS requests were previously running into a deadlock when trying to respond to requests in cases:

• To respond to the request, a lock needs to be obtained for the openssl TLS socket.
• However, meanwhile, this code is using that lock to try to read any subsequent requests.
• But no more requests are going to be sent by the browser until it gets a response to the first one!

This modifies it to wait for each request to finish responding until it goes on to reading the next request. It works, but it's not perfect; here are some things it doesn't do:

• It's potentially less parallelized with HTTPS than HTTP (but I'm not sure it's possible to do any better)
• Responding to requests out of order with HTTPS won't work (this could conceivably be fixed with some check-is-ready-to-read to supply more requests if they're available)

[luser]

@frewsxcv hey! We currently have a Cargo.toml patch in sccache to pick up this fix, which means that I find myself having to work around this to release a new version of sccache to crates.io. Do you think this PR is something that could land in the near future? I would appreciate it. Thanks!

[frewsxcv]

@luser I haven't had a lot of time to do FOSS work lately, and I don't use tiny-http anymore, and I also am not familiar enough with this particular code. If you're interested, I just added you to repo and gave you publish rights to this crate if you want to review + publish this

[tomaka]

Maybe the right solution would be to disable request pipelining altogether, even on non-HTTPS.

[luser]

Maybe the right solution would be to disable request pipelining altogether, even on non-HTTPS.

What would this look like? I'm not familiar enough with this crate to really even speculate.

[luser]

Mostly I'd like to get sccache back to using a non-patched version of this crate so we can release to crates.io without issues.

[mb64]

Just from a practical point of view, disabling pipelining altogether would be pretty infeasible; this PR is to some extent a workaround to disable it for HTTPS. I'm not super familiar with this library, but there's a lot of code dedicated to making sure the responses get sent in the right order, and other such difficulties that arise from it. Removing it altogether would warrant pretty major refactoring and changes to the basic architecture of the library.

Also, from a performance perspective, the way it currently works does, at least theoretically, make it faster, by allowing it to respond to earlier requests while also reading and responding to later requests. (This could be benchmark-able, but it would require a bunch of work.)

I'm not quite sure what's blocking this – is it a lack of reviewers, a hope for less workaround-y solutions, or something else? If it's the second one, I can try to implement a nicer thing that earns back the pipelining for HTTPS this weekend, if I have time.

FWIW, I've been using this library (with this change) for my own personal home webserver for the past few months, and haven't run into anything terrible. (To be fair, I don't really use it for anything, so it rarely gets traffic.)

[mb64]

Rebased to master

[chmanchester]

@tomaka, hi, are you able to review this change?

[tesuji]

@tiny-http Friendly ping for review

[karroffel]

I tried this out locally and it works fine, it would be nice to get this reviewed and in some way or the other fix the version on crates.io!

[tomaka]

This look good. I'm unfortunately not 100% certain of all the ramifications of this change, but I also can't think of anything that could go wrong from the top of my head.

[elichai]

Hi, could you publish a release please?