Re: [MirageOS-devel] Cohttp/Conduit refactoring update

[Anil Madhavapeddy <anil@xxxxxxxxxx>]

Thanks for the PR; reviewing now right after I get Async support in.  The 
Ocsigen tree is most helpful to see, thanks (and slightly scary ;-)

Some queries from a quick look:

- will Ocsigen also use the Cohttp header parsing?  That would simplify some of 
the functions right now (that force lowercase on comparisons, and parse various 
sub-protocols in headers).  Cohttp has an increasing number of parsers for the 
protocols within HTTP headers, although by no means complete.

- i notice there's lazy parsing of some fields like cookies.  I'd really like 
to make all the parsing in Cohttp lazy too (since most headers would never be 
accessed), but I expect that this will wait until 2.0 since it's a big 
interface change.

- Ocsigen_request_info.make is impressive :-)  I've only seen more function 
calls in autogenerated code in Xen's VM create function.

-a

On 28 May 2014, at 09:45, <romain.calascibetta@xxxxxxxxx> 
<romain.calascibetta@xxxxxxxxx> wrote:

> Hi all,
> 
> I'm interesting with last PR of Conduit and I try to integrate this with 
> Cohttp and then integrate with Ocsigenserver. Ocsigenserver follows in his 
> branch 3.0.0 the PR Cohttp#147 and Cohttp#143 (therefore Conduit#2, Conduit#5 
> and Conduit#4 for SSL support). But migration is more complex (~8000 
> additions and ~7000 deletions), you probably interest to look this file: 
> https://github.com/ocsigen/ocsigenserver/blob/3.0.0/src/server/ocsigen_cohttp_server.ml
>  (this file is main loop) and 
> https://github.com/ocsigen/ocsigenserver/blob/3.0.0/src/server/ocsigen_generate.ml(this
>  file is cast between Cohttp.Request and internal representation of request 
> Ocsigen_request_info).
> 
> The modifications of Conduit should not impact at top of layers (or, there is 
> minimal). And, I would be happy to change Ocsigenserver according to your PR.
> 
> Regards,
> 
> Romain Calascibetta - http://din.osau.re/
> 
> De : Anil Madhavapeddy
> Envoyà : âmercrediâ â28â âmaiâ â2014 â10â:â44
> Ã : Rudi Grinberg
> Cc : mirageos-devel@xxxxxxxxxxxxxxxxxxxx, romain.calascibetta@xxxxxxxxx
> 
> On 28 May 2014, at 02:08, Rudi Grinberg <rudi.grinberg@xxxxxxxxx> wrote:
> 
> > On Tue, 27 May 2014 17:11:45 -0400, Anil Madhavapeddy <anil@xxxxxxxxxx> 
> > wrote:
> > 
> >> Interesting -- I'd envisioned all that logic going into Conduit instead of 
> >> Cohttp itself.  The issue with Cohttp having all this logic is that it 
> >> can't be re-used easily by other protocol implementations, and it also 
> >> ties knowledge of IPv{4,6} into the HTTP library.  I believe Jon Ludlam 
> >> has some patches to send HTTP requests over shared memory vchan, which 
> >> would be difficult if Cohttp.Connection needs to be extended to know about 
> >> it.  Similarly, Arjun Guha submitted a domain socket mode so that he can 
> >> communicate with the Docker API via Cohttp: 
> >> https://github.com/mirage/ocaml-conduit/pull/3
> >> 
> >> With the Conduit patch, all this would be in that library instead.  
> >> Romain, do you have an Ocsigen working tree with your Conduit patch in 
> >> that I can take a look at?
> > 
> > I see. Reviewing the changes in conduit currently. That does seem like it 
> > would work much better.
> 
> Although it does make me wonder what the purpose of Cohttp.Connection really 
> is.  To be useful, it still needs a tie back to the underlying Conduit, but 
> also some info about which pipelined request it actually is.
> 
> > 
> >> 
> >> (re: pre and post 1.0 , the only patch I think needs to be deferred is the 
> >> completion of the module types from Lwt and Async moving out.  The rest 
> >> are all still pre 1.0 in my mind -- do you agree?)
> > 
> > That's fine with me. But note that we don't have to break any compatibility 
> > if we don't want to. We can always leave aliases to module signatures where 
> > they used to be. At least this was my plan originally.
> 
> Good point -- that would indeed avoid needless breakage.
> 
> -anil


_______________________________________________
MirageOS-devel mailing list
MirageOS-devel@xxxxxxxxxxxxxxxxxxxx
http://lists.xenproject.org/cgi-bin/mailman/listinfo/mirageos-devel