Skip to content
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.

Sign up for GitHub

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

Jump to bottom

cohttp-eio hangs with curl #883

Closed
talex5 opened this issue Jun 17, 2022 · 4 comments · Fixed by #929
Closed

cohttp-eio hangs with curl #883

talex5 opened this issue Jun 17, 2022 · 4 comments · Fixed by #929
Labels
Bug

Comments

@talex5
Copy link
Contributor

talex5 commented Jun 17, 2022

Using today's origin/master, curl hangs if you request a page from the example server that doesn't exist.

$ git describe 
v5.0.0-177-g628d8716

I ran the server with:

$ dune exec -- ./cohttp-eio/examples/server1.exe

Then ran the test client:

$ curl -v http://127.0.0.1:8080/missing
*   Trying 127.0.0.1:8080...
* Connected to 127.0.0.1 (127.0.0.1) port 8080 (#0)
> GET /missing HTTP/1.1
> Host: 127.0.0.1:8080
> User-Agent: curl/7.74.0
> Accept: */*
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 404 Not Found
* no chunk, no close, no size. Assume close to signal end
[ never finishes ]

It looks like curl is waiting for the connection to close, while cohttp is waiting for the next request. Perhaps there should be some size information sent for empty responses?

/cc @bikallem
@talex5 talex5 added the Bug label Jun 17, 2022
@bikallem
Copy link
Contributor

I may need to refresh my recollection of HTTP/1.1 spec but the connection is not supposed to be closed even after an error reply, no?

Will adding header Content-Length: 0 in the response be of any help?

@bikallem
Copy link
Contributor

In one of the earlier incarnations of cohttp-eio, it was adding the Content-Length and Date header. The view from other maintainers was that these headers are to be maintained/added by web apps themselves or other web frameworks that builds on top of cohttp-eio. As such it was removed. I think that is a sensible decision since webapps/frameworks may want to specialise certain responses, such as sending html response along with 401 status rather than just a 401 response.

@mseri
Copy link
Collaborator

mseri commented Jun 22, 2022

Just as a curiosity, what happens if you do the same curl -v http://127.0.0.1:8080/missing with any of the other cohttp servers?

@talex5
Copy link
Contributor Author

talex5 commented Jun 22, 2022

Lwt works:

$ dune exec -- ./bin/cohttp_server_lwt.exe &
$ curl -v http://localhost:8080/missing
*   Trying 127.0.0.1:8080...
* Connected to localhost (127.0.0.1) port 8080 (#0)
> GET /missing HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.74.0
> Accept: */*
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 404 Not Found
< content-length: 141
< 
* Connection #0 to host localhost left intact
<html><body><h2>Not Found</h2><p><b>/missing</b>was not found on this server</p><hr />Served by Cohttp/Lwt listening on :::8080</body></html>

The view from other maintainers was that these headers are to be maintained/added by web apps themselves or other web frameworks that builds on top of cohttp-eio. As such it was removed.

The example server is just using Cohttp_eio.Server.not_found_response. Seems like that should work, at least. Server checks Http.Request.is_keep_alive, but not Http.Response.is_keep_alive, although it doesn't look like that checks for a missing content-length header anyway.

@talex5 talex5 mentioned this issue Jul 7, 2022
Closed
@talex5 talex5 mentioned this issue Jul 28, 2022
Closed
@talex5 talex5 mentioned this issue Aug 24, 2022
Merged
4 tasks
@bikallem bikallem mentioned this issue Aug 31, 2022
Merged
@mseri mseri closed this as completed in #929 Sep 6, 2022
@craff craff mentioned this issue Dec 9, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

cohttp-eio: Add Content-Length header to request/response bikallem/ocaml-cohttp
3 participants
@bikallem @talex5 @mseri