Add Lwt UNIX adapter (httpaf-lwt-unix). #53
Conversation
|
Nice. I have another Lwt adapter locally. As part of it, I improved Faraday's Lwt |
|
@aantron That sounds good, is there also a reason to prefer your httpaf adaptor over this PR, or would that those be equivalent given the Faraday improvement? |
|
I haven't compared the two deeply enough to say, but I would expect our two adapters to be equivalent, and I'm happy to defer to yours and help review it. I can post mine as a PR for comparison, if you wish. The Faraday improvement should help your PR just as well as mine, it just consists of using Lwt's actual |
|
A parallel PR is not necessary on my behalf then, but can you send your PR to faraday? |
There was a problem hiding this comment.
The read loops in the client and server must check the return value of the read operation and buffer and unconsumed bytes for the next read. (I think this may actually be a problem in the async version as well. Gotta check that.)
Besides that, I think it looks good.
lwt-unix/httpaf_lwt_unix.ml
Outdated
| Lwt_bytes.read sock buffer 0 (Lwt_bytes.length buffer) >>= fun len -> | ||
| (if len = 0 | ||
| then Server_connection.shutdown_reader conn | ||
| else Server_connection.read conn buffer ~off:0 ~len |> ignore); |
There was a problem hiding this comment.
The server connection might not consume the entire buffer here. For example, if the input does not end on a token boundary. The code needs to check the return value of read and hold onto any bytes that were not consumed for the next call to read.
There was a problem hiding this comment.
I'll fix that and update the benchmarks post.
lwt-unix/httpaf_lwt_unix.ml
Outdated
| Lwt_bytes.read sock buffer 0 (Lwt_bytes.length buffer) >>= fun len -> | ||
| (if len = 0 | ||
| then Client_connection.shutdown_reader conn | ||
| else Client_connection.read conn buffer ~off:0 ~len |> ignore); |
There was a problem hiding this comment.
Same problem as in the server case.
|
Ah, right. It's not a problem in the async runtime. See the |
|
Just posted the alternative adapter in #54, which does have a |
|
The alternative adaptor PR was very helpful, thanks! I pushed a revised commit including:
I compared async and lwt adaptor of this PR using lwt-4.0.1, faraday-lwt-unix pinned to PR inhabitedtype/faraday#41, and the following parameters: For async, which was left with 401 failed connections after wrk exited, I got: and [updated after fixing reader threads,] for Lwt, which was left with 402 failed connections after wrk exited, I got: The lwt version fails with the following if it receives more than 1017 connections: I think this is due to a maximum |
paurkedal
force-pushed
the
lwt-unix
branch
3 times, most recently
from
b935fd7
to
13a41a7
Compare
|
@paurkedal, you should install |
|
Thanks, then I can present the same benchmark with Lwt: Now, there is actually a difference, the Lwt adaptor has a much more even latency distribution. |
|
The last two pushes:
But, I am worried though about the leaking connections after wrk is done. By repeated re-connects, I can drive up the number of connections reported by the benchmark server until it reaches the |
- Exit and relaunch reader and writer threads on `Yield. - Handle ENOTCONN, presumably needed when remote end shuts down first.
|
I push another commit which modifies the This does not solve the connection leak issue though. |
|
Just like in my comment in #54, I ran the benchmark for this PR locally with the following results: |
|
Thanks. I'll also do some benchmarking across different parameters and present a plot it in the afternoon. |
|
Here are my benchmarks:
The colors are blue for async, green for this PR, and red for #54. The three lines per color represent average, the average plus standard deviation, and the maximum. I used the Rec/Sec, rather than the one at the Requests/s at the bottom of the outputs. The parameters are the same as cited above, apart from the varying |
|
@seliopou I think this PR and #54 have pretty much converged in terms of features and correctness, so I recommend merging this one. I still sometimes see better performance from #54, but that could be because I am on WSL, and #54 is somehow implicitly over-optimized for my machine. Anyway, either PR can be easily tweaked later until it has the performance of the other, so this is not a big deal. Also, WSL is not important. @paurkedal I suggest rewriting the echo server using |
|
I updated the echo server and benchmark to use For good measures, here is the new benchmark results using the same setup as before: In case someone wants to reproduce the benchmarks, I published a gist containing the code. I noticed after writing it that wrk has scripting capabilities, so this could probably have been simplified. |
|
In case you want to merge this variant, the CI tests should recover after a rerun now that faraday has been updated. |
|
Indeed it does build now. Given that #54 supports buffering the input, I'm gonna close this PR in favor of that one. Thanks for opening this up and getting the ball rolling on lwt support! |
|
Sure! |
In some cases (especially when receiving a response with a close delimited body), the peer closes the connection before we attempt to SSL shutdown, leading to unnecessary reporting of errors such as `Unix.Unix_error(Unix.EBADF)`
Testing is limited to manual use use of the two provided examples (including with content greater than the buffer size).