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.

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

Cohttp #64

Closed
wants to merge 46 commits into from

Conversation

@Drup
Copy link
Member

commented Apr 2, 2015

Follows and is based on #63. This one contains the actual cohttp stuff. Starts after the commit 49f6433

@Drup

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2015

@dinosaure @rgrinberg It doesn't build, and I don't know cohttp enough to know why, please fix. :3

@Drup Drup force-pushed the cohttp_rebased branch from 3096cae to 248c8a4 Apr 2, 2015

@Drup

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2015

ok, thanks to @rgrinberg, it builds and launches now.

@rgrinberg

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2015

This frankenstein actually works O_o?

@rgrinberg

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2015

@Drup is your rebase magic great enough to kick out the bytes commits?

@Drup

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2015

It launches, as in make run.local do answer on localhost:8080. I'm not bold enough to say it works. ;)

What do you mean for the bytes commit ?
I already separated the bytes commit it two part, one in each PR. The other PR should be merged rather quickly, as it doesn't change any behavior, as far as I can see, just moving stuff in new files.

@rgrinberg

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2015

I mean that I'd like to give it another more thorough review but the diff is still a little heavy. I guess there's only so much magic you can do :/

@Drup

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2015

Are you looking commit by commit ? Apart from the big cohttp commit, the rest is small. And the big one is almost only the cohttp stuff. I didn't looked closely at the big commit yet.

@rgrinberg

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2015

@Drup at this point IMO it's a question of testing. I'm happy to help out but given that you're more experienced you should take the lead and make a plan for us.

@Drup Drup force-pushed the cohttp_rebased branch 4 times, most recently from c93143c to c075d52 Apr 3, 2015

@rgrinberg

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2015

@Drup @dinosaure Seems like there's a bug somewhere:

$ make run.opt.local
$ curl 'http://localhost:8080/ocsigenstuff/rpm.png'

Will not give you an image back.

This is where teh exception is thrown

| [] -> fail (Ocsigen_http_error (cookies_to_set, prev_err))

@rgrinberg

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2015

Looks like the above test only fails in cohttp master. 0.15.2 is working fine. We'll need to bisect through this stuff

mirage/ocaml-cohttp@v0.15.2...master

@dinosaure

This comment has been minimized.

Copy link
Member

commented Apr 4, 2015

I found the bug and it's very deep. In mirage:ocaml-cohttp@v0.15.2, the main conversion between Cohttp.Request and Ocsigen request in Ocsigen_generate receive this url if you go to the root of site web: http://127.0.0.1:5858/. But in mirage:ocaml-cohttp@master, this function receive: //127.0.0.1:5858/. It's possible to view the diff with a print_string on url at this line. The variable url is the result of composition between Uri.of_string and Request.uri.

I don't know if we change the compute of Cohttp or Ocsigen for this bug. I think, it's better to change the compute of Ocsigen.

@rgrinberg

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2015

@dinosaure good catch. Is this caused by the following re?

let url_re = Netstring_pcre.regexp "^([Hh][Tt][Tt][Pp][Ss]?)://([0-9a-zA-Z.-]+|\\[[0-9A-Fa-f:.]+\\])(:([0-9]+))?/([^\\?]*)(\\?(.*))?$" in

It doesn't allow url's to start without a scheme which is wrong.

@Drup Drup force-pushed the cohttp_rebased branch from c075d52 to 2f83209 Apr 4, 2015

@rgrinberg

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2015

@Drup @dinosaure OK so there's progress. This branch now works in the happy case. There are some differences in behavior for error handling however. For example:

$ curl 'http://0.0.0.0:8080/ocsigenstuff/'
# gives 404 on master
# gives the ugly 500 Error: Ocsigen_extensions.Ocsigen_http_error(_)
$ curl 'http://0.0.0.0:8080/index.html/'
# gives 500 on master
# gives the weird Error: Unix.Unix_error(Unix.ENOTDIR, "stat", # "/Users/rgrinberg/reps/ocsigen/ocsigenserver/local/var/www/index.html/") 
@rgrinberg

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2015

Backtraces:

ocsigenserver.opt: main: exception: Ocsigen_extensions.Ocsigen_http_error(_)
ocsigenserver.opt: main: backtrace:
ocsigenserver.opt: main: Raised at file "ocsigen_local_files.ml", line 179, characters 24-44
ocsigenserver.opt: main: Called from file "ocsigen_local_files.ml", line 187, characters 13-70
ocsigenserver.opt: main: Called from file "staticmod.ml", line 116, characters 5-73
ocsigenserver.opt: main: Called from file "staticmod.ml", line 130, characters 13-201
ocsigenserver.opt: main: Called from file "src/core/lwt.ml", line 685, characters 20-24
ocsigenserver.opt: main: exception: Unix.Unix_error(Unix.ENOTDIR, "stat", "/Users/rgrinberg/reps/ocsigen/ocsigenserver/local/var/www/index.html/")
ocsigenserver.opt: main: backtrace:
ocsigenserver.opt: main: Raised by primitive operation at file "ocsigen_local_files.ml", line 157, characters 15-43
ocsigenserver.opt: main: Called from file "staticmod.ml", line 116, characters 5-73
ocsigenserver.opt: main: Called from file "staticmod.ml", line 130, characters 13-201
ocsigenserver.opt: main: Called from file "src/core/lwt.ml", line 685, characters 20-24

@Drup Drup force-pushed the cohttp_rebased branch 3 times, most recently from 807f952 to e38170e Apr 5, 2015

@Drup

This comment has been minimized.

Copy link
Member Author

commented Apr 7, 2015

The eliom test runs, I'll paste the differences here:

  • in /uasuffix/2007/6
    • original: The suffix of the url is 2007/6, your user-agent is Mozilla/5.0 (X11; Linux x86_64; rv:37.0) Gecko/20100101 Firefox/37.0, your IP is ::1
    • cohttp: The suffix of the url is 2007/6, your user-agent is Mozilla/5.0 (X11; Linux x86_64; rv:37.0) Gecko/20100101 Firefox/37.0, your IP is 0.0.0.0
@Drup

This comment has been minimized.

Copy link
Member Author

commented Apr 7, 2015

The CRSF test is broken on both.

@Drup

This comment has been minimized.

Copy link
Member Author

commented Apr 7, 2015

alternative parameters seem to be broken too, on both.

@Drup

This comment has been minimized.

Copy link
Member Author

commented Apr 7, 2015

That's pretty much it. All the other things pass.

@vouillon

This comment has been minimized.

Copy link
Member

commented Apr 8, 2015

I noticed the following issues last month. We need to check that they are properly addressed:

  • Ocsigen_request_info.get_server_address is not implemented but is used by the reverse proxy;
  • the functions in the migrate directory should not raise Invalid_argument exceptions (more generally, a bogus HTTP client should not be able to trigger this kind of exception);
  • the Ocsigen_http_error exception carries a set of cookies to be sent back to the HTTP client, but these cookies are currently ignored;
  • we should probably remove the awake functions in Ocsigen_extensions.
@dinosaure

This comment has been minimized.

Copy link
Member

commented Apr 8, 2015

Thanks @vouillon for review and rebase, I'll work in your issues this Friday 👍. And specificaly for awake, I agree with you and it will be a ease for #52.

@Drup Drup force-pushed the cohttp_rebased branch from 764fdac to ec5119b Apr 8, 2015

rgrinberg and others added some commits May 10, 2015

@Drup Drup force-pushed the cohttp_rebased branch from 4dfed13 to 7a98d2f May 19, 2015

@Drup

This comment has been minimized.

Copy link
Member Author

commented on src/server/ocsigen_cohttp_server.ml in d5ca6c2 May 19, 2015

@vouillon @dinosaure This one seems logical, but the header was not propagated in the code before switching to cohttp. However, it is here. It's weird and I'm not really sure.

dinosaure added some commits Apr 10, 2015

Change the signature of Ocsigen_generate
In according with ocsigenserver@d4ae266. The compute of the [ADDR_INET]
will be in Ocsigen_cohttp_server module to compute only once.
@Drup

This comment has been minimized.

Copy link
Member Author

commented May 19, 2015

@dinosaure I pushed your (modified) stuff. I'm not very happy about the remaining assert false.

@Drup

This comment has been minimized.

Copy link
Member

commented on src/server/ocsigen_cohttp_server.ml in 5a1a9e0 May 19, 2015

This is a bit fishy and I feel like it's a landmine waiting to explode, but I don't know how to fix it. @vouillon, how does ocsigen did previously ?

@dinosaure

This comment has been minimized.

Copy link
Member

commented May 21, 2015

@Drup fix assert with d851d6f.

Remove assertion in Ocsigen_cohttp_server
Fix a misconception between the initialization of server in
Ocsigen_server and the main loop in Ocsigen_cohttp_server about the type
of address (before this patch, we cast an inet_addr to a string in
    initialization, and we cast this string (with possible fail) to
    inet_addr, now we keep inet_addr).
@Drup

This comment has been minimized.

Copy link
Member Author

commented May 21, 2015

@dinosaure Ok, I like this version much better. It's quite nice now.

Remove the awake parameter.
It's not needed anymore with cohttp.
@Drup

This comment has been minimized.

Copy link
Member Author

commented May 21, 2015

aaand I killed awake.

@Drup

This comment has been minimized.

Copy link
Member Author

commented May 21, 2015

I pushed 3 bug fixes. You can thanks warning 27.

@vouillon

This comment has been minimized.

Copy link
Member

commented Jul 23, 2015

HTTP/1.0 seems to be broken:

$ telnet localhost 8080
Trying ::1...
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.
GET / HTTP/1.0

HTTP/1.1 400 Bad Request
content-length: 43

Error: Ocsigen_lib_base.Ocsigen_Bad_Request
Connection closed by foreign host.
@vouillon

This comment has been minimized.

Copy link
Member

commented Jul 23, 2015

There is no buffering, so the reply can be extremely fragmented. But that's probably more an issue with Eliom/Tyxml than with Ocsigenserver.

6
<input
19
 placeholder="Your email"
10
 name="username"
d
 type="email"
3
 />
@mfp

This comment has been minimized.

Copy link
Contributor

commented Aug 18, 2015

While doing some benchmarks, I ran into this:

https://github.com/ocsigen/ocsigenserver/pull/64/files#diff-af00eddd5261fa6f6ab200223ead43f5R137

The Lwt_log.ign_debug_f is not removed/rewritten by pa_lwt_log, so the pretty printing (with Format!) is performed on every request. Merely changing to ignore begin ... Lwt_log.debug_f ... does not work even though the lwt.syntax.log is used because -syntax camlp4o isn't (maybe to prevent some ppx incompatibility).

Commenting it out gives over 30% speedup in smaller responses.

With this, the buffer pool in conduit, the last-chunk optimization and a change in the buffer size used by Ocsigen_senders.File_content, I've got cohttp_rebased within 5% of 2.6 with Lwt_chan fix + buffer pool + last-chunk opt (actually a bit faster at small responses, about the same speed in very large ones), or in other words, around 2X faster than stock 2.6 at large file transfer and ~30% at smaller responses.

@Drup

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2015

Except for the debug printer I hacked quickly (46fcee6), we are not using ppx so we should be able to enable camlp4. Thanks you for the pointer.

@vasilisp

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2017

Closing. We have revived the Cohttp effort in another branch. PR to come .

@vasilisp vasilisp closed this Jun 14, 2017

@vasilisp vasilisp deleted the cohttp_rebased branch Jun 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.