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

Async runtime: HTTPS support #92

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions async/buffer.ml
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
(** XXX(seliopou): Replace Angstrom.Buffered with a module like this, while
also supporting growing the buffer. Clients can use this to buffer and the
use the unbuffered interface for actually running the parser. *)
open Core
open Async

type t =
{ buffer : Bigstring.t
; mutable off : int
; mutable len : int }

let create size =
let buffer = Bigstring.create size in
{ buffer; off = 0; len = 0 }
;;

let compress t =
if t.len = 0
then begin
t.off <- 0;
t.len <- 0;
end else if t.off > 0
then begin
Bigstring.blit ~src:t.buffer ~src_pos:t.off ~dst:t.buffer ~dst_pos:0 ~len:t.len;
t.off <- 0;
end
;;

let get t ~f =
let n = f t.buffer ~off:t.off ~len:t.len in
t.off <- t.off + n;
t.len <- t.len - n;
if t.len = 0
then t.off <- 0;
n
;;

let put t ~f =
compress t;
let n = f t.buffer ~off:(t.off + t.len) ~len:(Bigstring.length t.buffer - t.len) in
t.len <- t.len + n;
n
;;

let put_async t ~f =
compress t;
f t.buffer ~off:(t.off + t.len) ~len:(Bigstring.length t.buffer - t.len)
>>= fun n ->
t.len <- t.len + n;
Deferred.return n
;;

10 changes: 10 additions & 0 deletions async/buffer.mli
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
open Core
open Async

type t

val create : int -> t

val get : t -> f:(Bigstring.t -> off:int -> len:int -> int) -> int
val put : t -> f:(Bigstring.t -> off:int -> len:int -> int) -> int
val put_async : t -> f:(Bigstring.t -> off:int -> len:int -> int Deferred.t) -> int Deferred.t
7 changes: 5 additions & 2 deletions async/dune
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
(library
(name httpaf_async)
(public_name httpaf-async)
(wrapped false)
(libraries
async core angstrom-async faraday-async httpaf)
async core angstrom-async faraday-async httpaf
(select ssl_io.ml from
(async_ssl -> ssl_io_real.ml)
(!async_ssl -> ssl_io_dummy.ml)))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not too familiar with dune conventions, but this feels like a bit of a poor user experience to me. If I understand correctly, it means that I can install httpaf-async, write a file that uses SSL that compiles fine, but at runtime I get an error about SSL not being available. What are our other options here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, it means that I can install httpaf-async, write a file that uses SSL that compiles fine, but at runtime I get an error about SSL not being available

That's correct. I agree this is not desirable.

I copied this almost line by line from conduit-async, but I think there are other options.

  1. I think it's valuable to let SSL be optionally installed, do you agree with this?
  2. We could probably make this fail at compile time if a user calls the function, like how Async.Printf doesn't compile and produces a type error of Probably_should_not_use_blocking_Core_Printf_functions_with_Async . If you agree this is a good enough solution I can implement it. Same for HTTPS support for the Lwt bindings #88.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I think it's valuable to let SSL be optionally installed, do you agree with this?

Not entirely, but if there are competing SSL implementations, it might be hard to avoid. I don't see a compelling argument for SSL as a feature being optional, at least.

  1. We could probably make this fail at compile time if a user calls the function

I'm thinking about a similar alternative where httpaf_async.mli exposes a module alias module Ssl = Ssl, and then if no ssl library is installed, that module is simply blank (or has some polymorphic variant stubs for documentation like Async.Printf); otherwise, it has Server and Client modules to mirror the ssl-less implementation. It also means you can simply open Httpaf_async.Ssl and get the interface you want.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be more explicit, the thing I wish we could express is "the user must have either library A or B installed, but if neither exists then install A as a dependency". I would be surprised if that was possible, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be more explicit, the thing I wish we could express is "the user must have either library A or B installed, but if neither exists then install A as a dependency". I would be surprised if that was possible, though.

I don't think this is possible to express. Not that I have seen, at least.

Not entirely, but if there are competing SSL implementations, it might be hard to avoid. I don't see a compelling argument for SSL as a feature being optional, at least.

My point is that there are systems where people have trouble building either SSL or TLS (I've heard reports from Windows). My goal with the current state of this PR was to still have the library build (e.g. if people are just using the Server module and deploying behind a reverse proxy).

I'm thinking about a similar alternative where httpaf_async.mli exposes a module alias module Ssl = Ssl, and then if no ssl library is installed, that module is simply blank

I don't follow 100%, but I can put together a proposal soon for you too look at.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're currently porting the OCaml TLS stack to be fully Dune'd up, and it also has Async bindings. The original decision in Conduit-async was because we weren't sure of the maturity of OCaml TLS vs OpenSSL then, but it's worked out pretty well in OCaml TLS' favour since.

Once the deps are all Duned up, Windows support should be much easier to guarantee...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One reason why I currently use ocaml-ssl over ocaml-tls is because of lacking support for elliptic curve ciphersuites in the latter. That was my motivation for including both implementations in this PR.

(modules buffer httpaf_async ssl_io)
(flags (:standard -safe-string)))
Loading