Client support for receiving 1xx informational responses

[seanmonstar]

This is to discuss adding support for accessing 1xx informational responses on the client side (see #2426 for server side). Currently, hyper parses all 1xx responses and ignores them, as RFC 7230 says it must at least be able to receive and ignore them.

The Client (and client::conn) interfaces return an impl Future<Output = Result<Response>>, which only allows awaiting it for a single response. We can't really add new methods to allow also awaiting for informational responses, unless we stuck to always return a specialized type that is both a Future and has extra methods. However, using those together wouldn't be that clear to a user. You can't be sure if or how many informational responses would be received before the full response, so you would want to await "both" types.

Instead, we can use a callback format. We could define a type (hyper::client::ext::Informational? On1xx? names...), and the user would store that in the request extensions. We'd then check for that in the h1 proto code and if it exists, call the callback with the 1xx headers.

[nox]

I much prefer the idea of adding methods on ResponseFuture, hooks make bookkeeping harder for the caller, and I don't think it is more confusing to have both those methods and the Future impl than say, using poll_data and poll_trailers on HttpBody.

[bagder]

In the curl project, there's right now 105 test cases disabled when built with hyper. 46 of them use 100-continue in one way or another and we really need to know when we get 1xx responses (or not) to make curl+hyper behave like the built-in HTTP code does. It could allow us to make significant progress in my task to make curl+hyper match the built-in HTTP code.

[seanmonstar]

Right-o, I got distracted with other work. I'll do some thinking and work on this in the next few days.

[seanmonstar]

I've spent some time researching options (prior art of APIs), and considering the existing code, and I think I've settled on staged approach. I agree with you, @nox, we can add a method to ResponseFuture on the Rust side, and make it work right. However, it'd be less easy to access in the exposed C API, and will take more time to get right. So:

1. First, add support for a callback extension, which will be exposed in the C API. This is the least complicated to do on hyper's side, so we can do it quickly. Importantly, though, most of it can be reused or only slightly altered to support the next steps.
2. Because of the current client design, which separates the ResponseFuture from the separate client dispatcher task running in the background, we'd need some to add some sort of channel where the receiver is inside ResponseFuture, and the sender is in a request extension. Instead of storing the sender directly, we can reuse the callback to simply store a closure sending on the sender side of the channel.
3. Finally, the higher level ResponseFuture (returned by hyper::Client, not hyper::client::conn) would need to separate some of its stages into an internal enum, so that polling ResponseFuture::poll_informational() could advance the stages far enough that the task wouldn't be stuck since the dns/connect/write stage hadn't gotten far enough.

---

The first step is my next work priority.

[seanmonstar]

Step 1 PR is available at #2594.

[seanmonstar]

@bagder the piece you'd need is merged to master now. hyper's C API doesn't use timers at the moment, so you'd need to use a timer in the normal curl way, and check in the body poll callback whether a 100 Continue has been received (in your informational callback, you could set a flag and trigger the body waker to start polling again) or the timer has expired.

[bagder]

I would've liked the callback to take a return code so that we can return error easier.

If my code in the callback runs into a fatal error, such as out of memory, I need to return error and abort everything. Instead of returning error, now I need to pass that info back to the parent caller by storing a magic value in a struct field instead. Not your typical erroring out style code.

[bagder]

What's perhaps worse, is that it doesn't seem to work? At least not the way I thought it is supposed to work. Shouldn't the following callback code be able to extract the 100 response and its associated response headers?

When I run this in curl test 158, the header callback doesn't get invoked even once. I suppose I've done something wrong?

https://github.com/curl/curl/blob/931a3e33e11c75feca367b90532d95d6f17985f7/lib/c-hyper.c#L726-L750

[seanmonstar]

I would've liked the callback to take a return code so that we can return error easier.

We could probably adjust it to be more like the foreach functions, returning a HYPERE_ABORTED_BY_CALLBACK if you return a non-zero.

Shouldn't the following callback code be able to extract the 100 response and its associated response headers?

It should... I added it's usage to the upload.c example, and checked it worked there. I'll dig a little into how you're using it.

[seanmonstar]

I noticed a difference in your test case, which is that it seems it's only sending the 100 Continue, and not sending a 200 OK afterwards, right? When I make a local server doing the same thing, I still see the capi/examples/upload.c callback trigger, but don't see it print about the final response, which makes sense.

[bagder]

Yes, the test only responds with a 100 and then closes the connection. But there's something else going on here.

Here's what I do:

response

I now made the HTTP server respond this exact byte stream. I did it by editing my local curl test 158

HTTP/1.1 100 Continue swsclose
Silly-header: yeeeees

HTTP/1.1 200 OK
Server: moo
Content-Length: 0

(with CRLF newlines)

test server

$ ./server/sws

I run the test server in curl's tests/ directory like this, and then it runs a stupid HTTP server foir IPv4 on port 8999. It loads test case based on the file name part in the URL. I put 158 there to make it load contents from test 158, which I have edited as above.

upload example

I then run the hyper capi "upload" test program against this test server URL.

I fire away the upload example to get to see the response codes and the headers:

$ ./upload upload.c localhost 8999 /here/158
connecting to port 8999 on localhost...
connected to localhost, now upload to /here/158
http handshake ...
preparing http request ...
sending ...

Response Status: 200
Server: moo
Content-Length: 0

There's no Informational (1xx): output there...

[seanmonstar]

Something very strange indeed... For instance, when I have a local server doing the same thing, I get this output:

$ ./upload ../README.md 127.0.0.1 3000 /up
connecting to port 3000 on 127.0.0.1...
connected to 127.0.0.1, now upload to /up
http handshake (hyper v0.14.11) ...
preparing http request ...
    with expect-continue ...
sending ...

Informational (1xx): 100
HTTP/1.1 100 Continue swsclose
Silly-Header: yeeesss


Response Status: 200
Content-Length: 5

hello
 -- Done! --

It could something weird inside hyper, but a possible idea popped into my head: has the rust library been re-compiled after pulling from git? Just for my sanity, I went ahead and added the hyper_version() output to the examples in #2623.

[bagder]

Spot on. A simple make in the examples dir didn't rebuild upload...

[bagder]

I can reproduce my problem with this patch to upload.c:

index fa7134f3..acf9bfbb 100644
--- a/capi/examples/upload.c
+++ b/capi/examples/upload.c
@@ -151,12 +151,12 @@ static int print_each_header(void *userdata,
 static void print_informational(void *userdata, const hyper_response *resp) {
     uint16_t http_status = hyper_response_status(resp);
 
     printf("\nInformational (1xx): %d\n", http_status);
 
-    const hyper_buf* headers = hyper_response_headers_raw(resp);
-    write(1, hyper_buf_bytes(headers), hyper_buf_len(headers));
+    hyper_headers* headers = hyper_response_headers((hyper_response *)resp);
+    hyper_headers_foreach(headers, print_each_header, NULL);
 }
 
 typedef enum {
     EXAMPLE_NOT_SET = 0, // tasks we don't know about won't have a userdata set
     EXAMPLE_HANDSHAKE,

[seanmonstar]

Ah, with that change, I still see the callback called, I see Informational (1xx): 100\n, but then no headers are printed. (I also notice the C compiler yelling at me about const pointers...) Let me see what's going on internally...

[seanmonstar]

Found the issue with the missing headers. I also took the opportunity to fix up that const warning.

[bagder]

then no headers are printed

Right, that's exactly the problem I saw.

[bagder]

Now also confirmed with curl test 158 working fine in my end. 🎉