-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Client support for receiving 1xx informational responses #2565
Comments
I much prefer the idea of adding methods on |
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. |
Right-o, I got distracted with other work. I'll do some thinking and work on this in the next few days. |
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
The first step is my next work priority. |
Step 1 PR is available at #2594. |
@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. |
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. |
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 |
We could probably adjust it to be more like the foreach functions, returning a
It should... I added it's usage to the |
I noticed a difference in your test case, which is that it seems it's only sending the |
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: responseI now made the HTTP server respond this exact byte stream. I did it by editing my local curl test 158
(with CRLF newlines) test server
I run the test server in curl's upload exampleI 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:
There's no Informational (1xx): output there... |
Something very strange indeed... For instance, when I have a local server doing the same thing, I get this output:
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 |
Spot on. A simple |
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, |
Ah, with that change, I still see the callback called, I see |
Found the issue with the missing headers. I also took the opportunity to fix up that |
Right, that's exactly the problem I saw. |
Now also confirmed with curl test 158 working fine in my end. 🎉 |
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 animpl 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 aFuture
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.The text was updated successfully, but these errors were encountered: