cohttp-eio: improve server API

[talex5]

This is a big PR with various changes. If there are objections, I can split it into smaller patches.

On my machine, wrk shows this version is slightly faster (before: 169.30 MB/s, after 177.89 MB/s, testing with wrk -t 4 -c 1000 -d 5s http://127.0.0.1:8080 and the example server).

• Use Eio.Buf_write from Eio 0.3 for writing.

• Add Request module and make type abstract.
  This allows adding extra fields without breaking user code.
  It also makes the sample code simpler.

• Add client address to Request.t.
  This is allows writing logging middleware.

• Provide Server.connection_handler, to let people use their own accept loop if they prefer.
  This is also useful for unit-tests.

• Remove switch argument from Server.run. Taking a switch makes it look like the function create resources that outlive it, but it doesn't, so there's no need for this. Also, change the return type to 'a to show that the function never returns.

• Use Logs library for logging.
  Log server errors instead of writing to stderr.
  Log parsing errors instead of throwing them away.
  However, this does mean that you don't see errors at all until logging is configured, but this is a problem with the Logs library rather than with cohttp.

• Add some test cases using the new Eio_mock library.

This is on top of #886.

/cc @bikallem

[bikallem]

Use Eio.Buf_write from Eio 0.3 for writing.

Nice!

Add Request module and make type abstract.
This allows adding extra fields without breaking user code.
It also makes the sample code simpler.

cohttp-eio had Request module at some point. However, this was removed:

1. The consensus was to use/expose Http.Request.t rather than wrap them.
2. Request.read_fixed and Request.read_chunked doesn't make much sense in the Client module where we need to do the opposite, i.e. write_fixed and write_chunked. Once we remove those two functions, the Request module is mostly just a wrapper for Http.Request.t, so we might as well just use Http.Request.t.
3. Because of (2), Request module is only really usable with servers and not clients.

Therefore at this stage I'd rather not re-introduce Request module.

Use Logs library for logging.
Log server errors instead of writing to stderr.
Log parsing errors instead of throwing them away.
However, this does mean that you don't see errors at all until logging is configured, but this is a problem with the Logs library rather than with cohttp.

Add some test cases using the new Eio_mock library.

Nice!

The rest seems okay to me.

EDIT: Regarding the Request module, perhaps the abstraction benefit is worth it (in terms of code evolution) . @mseri what do you think?

[mseri]

Hi, apologies but I will not able to have a proper look before one (or even two) weeks. @rgrinberg can you have a look?

[talex5]

Request.read_fixed and Request.read_chunked doesn't make much sense in the Client module where we need to do the opposite

This is a good point. Perhaps we could have Server.Request? In any case, I think the client address needs to be exposed somehow for logging, so if we don't abstract it then we should expand the tuple or use a record.

[bikallem]

This is a good point. Perhaps we could have Server.Request? In any case, I think the client address needs to be exposed somehow for logging, so if we don't abstract it then we should expand the tuple or use a record.

Yes, let's expand the tuple with client_address for now. We can revisit this later if the tuple contains too many items.

[rgrinberg]

I think the client address needs to be exposed somehow for logging, so if we don't abstract it then we should expand the tuple or use a record.

Is the host header not enough?

[rgrinberg]

We can revisit this later if the tuple contains too many items.

If you plan to change the API, you should at least mark it as unstable for now.

[rgrinberg]

Can you drop everything to do with Logs? This is a mistake we've made with the original library and I hate to see it repeated here. Let's leave logging to the user and think of an interface that allows the user to pick their own implementation. If you feel very strongly about a particular implementation, make a framework and introduce it there.

[talex5]

Yes, let's expand the tuple with client_address for now. We can revisit this later if the tuple contains too many items.

OK, I've removed the Request module for now, though I think the API was better with it.

Is the host header not enough?

The host header says where the request is going, not where it came from.

Can you drop everything to do with Logs?

OK, done. Now that connection_handler and the client address are exposed, the user can provide their own logging easily enough.

[rgrinberg]

The host header says where the request is going, not where it came from.

Yes, sorry. But the problem with exposing the client address remains: you're inviting users to write servers that will break proxies. What about introducing something higher level that consults the various proxy headers? This seems like it will be good enough for logging at least.

[talex5]

Yes, sorry. But the problem with exposing the client address remains: you're inviting users to write servers that will break proxies. What about introducing something higher level that consults the various proxy headers? This seems like it will be good enough for logging at least.

That doesn't make sense to me:

1. cohttp should report the HTTP headers as what the user sent, without changes. Other libraries can be built on top to do other things.
2. If cohttp always replaces the user's source header with the actual source address then proxies are still just as broken, because the cohttp on the server will replace the header that was set by cohttp on the proxy.
3. If cohttp doesn't replace the user's header, then the user can spoof their source address.
4. Storing security information about the user in the same place as untrusted data supplied by the user themselves seems like a bad idea in general.

[talex5]

I've rebased this on master and updated to Eio 0.4 (which just removes the need to flush the writer on error). I plan to merge it tomorrow unless someone objects (it's been over a month since the last comment).