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

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

This frankenstein actually works O_o?

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

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.

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 :/

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.

@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 @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

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

I found the bug and it's very deep. In mirage:[email protected], 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.

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

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

@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/") 

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

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

The CRSF test is broken on both.

alternative parameters seem to be broken too, on both.

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

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.

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.

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

@Drup fix assert with d851d6f.

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

aaand I killed awake.

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

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.

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
 />

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.

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.

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