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

Upstream Jane Street Changes #227

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 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
3 changes: 3 additions & 0 deletions lib/client_connection.ml
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,15 @@
POSSIBILITY OF SUCH DAMAGE.
----------------------------------------------------------------------------*)

open Sexplib.Std

module Reader = Parse.Reader
module Writer = Serialize.Writer

module Oneshot = struct
type error =
[ `Malformed_response of string | `Invalid_response_body_length of Response.t | `Exn of exn ]
[@@deriving sexp_of]

type response_handler = Response.t -> Body.Reader.t -> unit
type error_handler = error -> unit
Expand Down
4 changes: 2 additions & 2 deletions lib/dune
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
(library
(name httpaf)
(public_name httpaf)
(libraries
angstrom faraday bigstringaf)
(libraries angstrom faraday bigstringaf base sexplib)

Choose a reason for hiding this comment

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

Do we really need sexplib here?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, for the [@deriving sexp] on the types. However, I don't think we need the base dependency, so I've removed it.

Choose a reason for hiding this comment

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

Hmmhmm, but do we really need [@deriving sexp]?

It is of very limited interest. As far as I can see, there is no internal use for the deriver in the project - so it responds to an external demand (I suppose, in this case, JaneStreet). However, there is no demand (in the issues) for such a deriver and I'm not personally in favour of using it.

Copy link
Author

@TyOverby TyOverby Sep 11, 2023

Choose a reason for hiding this comment

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

As far as I can see, there is no internal use for the deriver in the project - so it responds to an external demand

They're used in some expect_tests that I'm still in the process of upstreaming.

so it responds to an external demand (I suppose, in this case, JaneStreet).

All of the people who have contributed to httpaf currently work at Jane Street, so the internal / external divide isn't totally clear. I also won't pretend to speak on their behalf, I'm just doing the work of pushing the changes that they already made.

Hmmhmm, but do we really need [@deriving sexp]?

The dependency is very small, and if you have code that uses the sexp deriver, then having the serializers and deserializers directly in the library means that you don't have to re-declare the types to add the derivers and keep them in sync.

Choose a reason for hiding this comment

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

All of the people who have contributed to httpaf currently work at Jane Street

That's a little simplistic...

I also won't pretend to speak on their behalf, I'm just doing the work of pushing the changes that they already made.

There are several questions here:

  1. how do maintainers/authors view httpaf and its community.
  2. JaneStreet surely has an internal use of httpaf but it seems that not all of it has been exported here (furthermore, Support upgrades via I/O operations #159 or Http upgrades #203 exist and do not add the deriver)
  3. httpaf is used by other projects (notably dream) for which adding a dependency (even a small one) has real implications.

For example, as far as Mirage is concerned, we've already had problems with sexplib0 (which we've of course fixed).

The dependency is very small, and if you have code that uses the sexp deriver, then having the serializers and deserializers directly in the library means that you don't have to re-declare the types to add the derivers and keep them in sync.

This remains a very specific use. Once again, no one in the community has asked for such a deriver, even if it's only in the issues. It should also be understood that it can be frustrating to have a dependency imposed on you as a long-standing user (and contributor in a way - as far as faraday, angstrom or bigstringaf are concerned) of httpaf for a reason that is as obscure as it is internal to JaneStreet.

(preprocess (pps ppx_sexp_conv))
(flags (:standard -safe-string)))
11 changes: 8 additions & 3 deletions lib/headers.ml
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,14 @@
POSSIBILITY OF SUCH DAMAGE.
----------------------------------------------------------------------------*)

open Sexplib.Std

type name = string
type value = string
type t = (name * value) list

type name = string [@@deriving sexp]
type value = string [@@deriving sexp]
type t = (name * value) list [@@deriving sexp]

let sexp_of_t t = sexp_of_t (List.rev t)

let empty : t = []

Expand Down Expand Up @@ -70,6 +74,7 @@ module CI = struct
done;
!equal_so_far
)
;;
end

let ci_equal = CI.equal
Expand Down
2 changes: 1 addition & 1 deletion lib/headers.mli
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
type t
type t [@@deriving sexp]

type name = string
type value = string
Expand Down
28 changes: 26 additions & 2 deletions lib/httpaf.mli
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@
1.1 specification, and the basic principles of memory management and
vectorized IO. *)



(** {2 Basic HTTP Types} *)


Expand All @@ -58,6 +60,7 @@ module Version : sig
{ major : int (** The major protocol number. *)
; minor : int (** The minor protocol number. *)
}
[@@deriving sexp]

val compare : t -> t -> int

Expand Down Expand Up @@ -95,12 +98,14 @@ module Method : sig
| `TRACE
(** {{:https://tools.ietf.org/html/rfc7231#section-4.3.8} RFC7231§4.3.8}. Safe.*)
]
[@@deriving sexp]

type t = [
| standard
| `Other of string
(** Methods defined outside of RFC7231, or custom methods. *)
]
[@@deriving sexp]

val is_safe : standard -> bool
(** Request methods are considered "safe" if their defined semantics are
Expand Down Expand Up @@ -147,6 +152,7 @@ module Status : sig
| `Continue
| `Switching_protocols
]
[@@deriving sexp]
(** The 1xx (Informational) class of status code indicates an interim
response for communicating connection status or request progress
prior to completing the requested action and sending a final
Expand All @@ -164,6 +170,7 @@ module Status : sig
| `Reset_content
| `Partial_content
]
[@@deriving sexp]
(** The 2xx (Successful) class of status code indicates that the client's
request was successfully received, understood, and accepted.

Expand All @@ -179,6 +186,7 @@ module Status : sig
| `Use_proxy
| `Temporary_redirect
]
[@@deriving sexp]
(** The 3xx (Redirection) class of status code indicates that further
action needs to be taken by the user agent in order to fulfill the
request.
Expand Down Expand Up @@ -209,6 +217,7 @@ module Status : sig
| `I_m_a_teapot
| `Enhance_your_calm
]
[@@deriving sexp]
(** The 4xx (Client Error) class of status code indicates that the client
seems to have erred.

Expand All @@ -223,6 +232,7 @@ module Status : sig
| `Gateway_timeout
| `Http_version_not_supported
]
[@@deriving sexp]
(** The 5xx (Server Error) class of status code indicates that the server is
aware that it has erred or is incapable of performing the requested
method.
Expand All @@ -237,11 +247,13 @@ module Status : sig
| client_error
| server_error
]
[@@deriving sexp]
(** The status codes defined in the HTTP 1.1 RFCs *)

type t = [
| standard
| `Code of int ]
[@@deriving sexp]
(** The standard codes along with support for custom codes. *)

val default_reason_phrase : standard -> string
Expand Down Expand Up @@ -320,7 +332,7 @@ end
See {{:https://tools.ietf.org/html/rfc7230#section-3.2} RFC7230§3.2} for
more details. *)
module Headers : sig
type t
type t [@@deriving sexp]

type name = string
(** The type of a case-insensitive header name. *)
Expand Down Expand Up @@ -447,6 +459,9 @@ module Body : sig
module Reader : sig
type t

val create : Bigstringaf.t -> t
(** [create bs] creates a [t] using [bs] as the internal buffer. *)

val schedule_read
: t
-> on_eof : (unit -> unit)
Expand All @@ -468,6 +483,9 @@ module Body : sig
val is_closed : t -> bool
(** [is_closed t] is [true] if {!close} has been called on [t] and [false]
otherwise. A closed [t] may still have bytes available for reading. *)

val unsafe_faraday : t -> Faraday.t
(** [unsafe_faraday t] retrieves the raw Faraday object from [t]. Unsafe. *)
end

module Writer : sig
Expand Down Expand Up @@ -527,6 +545,7 @@ module Request : sig
; target : string
; version : Version.t
; headers : Headers.t }
[@@deriving sexp]

val create
: ?version:Version.t (** default is HTTP 1.1 *)
Expand Down Expand Up @@ -573,6 +592,7 @@ module Response : sig
; status : Status.t
; reason : string
; headers : Headers.t }
[@@deriving sexp]

val create
: ?reason:string (** default is determined by {!Status.default_reason_phrase} *)
Expand Down Expand Up @@ -656,6 +676,8 @@ module Reqd : sig
val respond_with_bigstring : t -> Response.t -> Bigstringaf.t -> unit
val respond_with_streaming : ?flush_headers_immediately:bool -> t -> Response.t -> Body.Writer.t

val respond_with_upgrade : ?reason:string -> t -> Headers.t -> unit

(** {3 Exception Handling} *)

val report_exn : t -> exn -> unit
Expand Down Expand Up @@ -697,7 +719,7 @@ module Server_connection : sig
(** [create ?config ?error_handler ~request_handler] creates a connection
handler that will service individual requests with [request_handler]. *)

val next_read_operation : t -> [ `Read | `Yield | `Close ]
val next_read_operation : t -> [ `Read | `Yield | `Close | `Upgrade ]
(** [next_read_operation t] returns a value describing the next operation
that the caller should conduct on behalf of the connection. *)

Expand All @@ -724,6 +746,7 @@ module Server_connection : sig
val next_write_operation : t -> [
| `Write of Bigstringaf.t IOVec.t list
| `Yield
| `Upgrade
| `Close of int ]
(** [next_write_operation t] returns a value describing the next operation
that the caller should conduct on behalf of the connection. *)
Expand Down Expand Up @@ -774,6 +797,7 @@ module Client_connection : sig

type error =
[ `Malformed_response of string | `Invalid_response_body_length of Response.t | `Exn of exn ]
[@@deriving sexp_of]

type response_handler = Response.t -> Body.Reader.t -> unit

Expand Down
3 changes: 3 additions & 0 deletions lib/method.ml
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
POSSIBILITY OF SUCH DAMAGE.
----------------------------------------------------------------------------*)

open Sexplib.Std

type standard = [
| `GET
Expand All @@ -42,11 +43,13 @@ type standard = [
| `OPTIONS
| `TRACE
]
[@@deriving sexp]

type t = [
| standard
| `Other of string
]
[@@deriving sexp]

let is_safe = function
| `GET | `HEAD | `OPTIONS | `TRACE -> true
Expand Down
2 changes: 1 addition & 1 deletion lib/parse.ml
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ let response =
lift4 (fun version status reason headers ->
Response.create ~reason ~version ~headers status)
(version <* char ' ')
(status <* char ' ')
(status <* option ' ' (char ' '))
(take_till P.is_cr <* eol <* commit)
(headers <* eol)

Expand Down
Loading