HTTP/2

[jamesdbrock]

• New module HTTP2 with low-level Effect bindings and high-level Aff bindings. Solves Add support for http2 #44.
• Use spec for tests. Upgraded ci.yml. Solves Ensure that tests terminate #35.
• New function HTTP.onRequest. Solves HTTP.createServer then close after one connection #46.
• Add spago.dhall.

The main new feature of this PR is HTTP/2 support. The main theme of this PR is “do all network I/O in the Aff monad.” The HTTP/2 Effect bindings are selected mostly because they are necessary to write the Aff API. Most of the documentation focuses on the Aff API.

This PR is not a complete set of bindings to https://nodejs.org/docs/latest/api/http2.html , but it lays down what is in my opinion the correct foundation of vocabulary types and Aff idioms.

If you want to understand what this PR is about, the best way to start is to clone the repo, spago docs, and read the module documentation for Node.HTTP2, Node.HTTP2.Server.Aff, and Node.HTTP2.Client.Aff.

I expect the newtype OptionsObject = OptionsObject Foreign in Node.HTTP2 might be controversial. The Node.HTTP API is written in terms of Data.Options. Instead of writing the Node.HTTP2 API in terms of Data.Options, I treat the options object as a Foreign with a newtype wrapper. Users and downstream libraries can write their own Option types and use options to construct the Foreign options object.

I added a spago.dhall; that might also be controversial. None of the other purescript-node packages have a spago.dhall. But spago is the only tool I know how to use, and also, with the imminent Registry change, all packages will soon have a spago.dhall, right?

UPDATE 2022-12-05

This PR does not support a listening server that can handle both HTTP/2 and HTTP/1.x. For that, apparently, we need to use a completely different Node.js API for the HTTP/2 server; the “Compatibility API”.

The Compatibility API requires us to listen for 'request' events instead of 'stream' events, and then in the style of Node.js http mutate a Response object which is passed to the 'request' handler.

---

Checklist:

• Added the change to the changelog's "Unreleased" section with a reference to this PR (e.g. "- Made a change (#0000)")
• Linked any existing issues or proposals that this pull request should close
• Updated or added relevant documentation
• Added a test for the contribution (if applicable)

[jamesdbrock]

Possible future worK:

• Trailers testing -- Testing all of the stuff with trailing headers. Done.
• ALPN -- Testing HTTP/1 with the HTTP2 module, et cetera. ALPN cannot work with the code in this PR. See note in PR description.
• A different treatment for Options? Maybe https://github.com/natefaubion/purescript-convertable-options ?
• Break up spago.dhall into a special spago.dhall for testing? Done.

[jamesdbrock]

Some PureScript HTTP servers

• https://github.com/sigma-andex/purescript-httpurple
• https://github.com/citizennet/purescript-httpure
• https://github.com/hoodunit/purescript-payload
• https://github.com/purescript-hyper/hyper
• https://github.com/bklaric/purescript-idiomatic-node-http

[JordanMartinez]

I don't think this is the place to write a small tutorial on Aff. I think that is better served by people clicking on the Aff type and reading any docs on that type.

[jamesdbrock]

I don't think this is the place to write a small tutorial on Aff.

Beginners are going to need this explanation. Maybe I should move this into the docs directory?

[jamesdbrock]

I've learned there is no docs directory for any of the purescript-node repos. I’d rather decide whether to merge this PR and then move the docs around later.

[thomashoneyman]

I agree with @JordanMartinez that this isn't the right place for an Aff tutorial, but it's a fine place to concretely demonstrate using this module via Aff.

[JordanMartinez]

Meaning, I think these docs should be moved to Aff specifically, and then the module header above may say, "Refer to Aff for more details. Here's a few examples of what you likely want." Aff isn't well-documented in the first place aside from Nate's video on it and my learning repo which summarizes Nate's video in text/code.

[jamesdbrock]

I think these docs should be moved to Aff specifically, and then the module header above may say, "Refer to Aff for more details. Here's a few examples of what you likely want."

I love that idea but let's do it after this PR.

[jamesdbrock]

Question: Should the asynchronous functions be type forall m. MonadAff m => m a instead of Aff a?

[JordanMartinez]

I think the FFI should be using types like EffectFnX instead of the curried variants. The code will optimized better with purs-backend-es that way (either now or in the future). So, instead of writing something like:

foreign import onStream :: Foreign -> (stream -> headers -> flags -> Effect Unit) -> Effect Unit

export const onStream = foreign => callback => () => {
  const cb = (stream, headers, flags) => callback(stream)(headers)(flags)();
  foreign.on("stream", cb);
  return () => {foreign.removeListener("stream", cb);};
};

we'd write

foreign import onStreamImpl :: EffectFn2 Foreign (EffectFn3 stream headers flags Unit) (Effect Unit)
onStream :: Foreign -> (stream -> headers -> flags -> Effect Unit) -> Effect (Effect Unit)
onStream f cb = runFn2 onStreamImpl f (mkEffectFn3 cb)

export const onStream = (foreign, callback) => {
  const cb = (stream, headers, flags) => callback(stream)(headers)(flags)();
  foreign.on("stream", callback);
  return () => foreign.removeListener("stream", callback);
};

[JordanMartinez]

The file path for this file should changed:

-src/Node/HTTP2/Client.Aff.purs
+src/Node/HTTP2/Client/Aff.purs

[jamesdbrock]

Really? I like this way. But if you insist, I'll make that change.

[jamesdbrock]

I just went to make that change and now I remember why I don't like this: because then we end up with two files named Aff.purs. Which is not a good name for a file. And that convention gets worse as we scale up; if a repo has a lot of Effect binding modules with a lot of Aff API modules to go with them, then we just keep adding more files named Aff.purs.

Is there some tooling reason why we would prefer to accumulate Aff.purs files? Some tool which requires module prefixes to be subdirectories?

[JordanMartinez]

There's nothing stopping someone from using qualified imports like

import Node.Http2.Client.Aff as Client.Aff

[JordanMartinez]

Really? I like this way. But if you insist, I'll make that change.

This just goes against convention with how everything else is done in core/contrib/web/node libs. I also wonder if this would break things in purs-backend-es if we did go with this approach and someone defined a module using the convention and exposed a similarly named function defined here.

[jamesdbrock]

Ok done.

[JordanMartinez]

Can you clarify why the Node.HTTP2.Internal module exists? It seems like you write FFI once that reuse it in both the client and server files. Was that to reduce the amount of code that needed to be written? Or is there some other reason? If the HttpServer' connectionType design is utilized and that design is used in other places (e.g. ClientHttp2Session/ServerHttp2Session -> Http2Session clientOrServerType), would the module still be needed? Or put differently, could the module instead be called Shared.purs which indicates its functions can be used by either the client or server?

[JordanMartinez]

Should this be an ADT?

[jamesdbrock]

I don't think so? Those integer codes come from deep inside Nghttp2 and I think that if we constrain them with the PureScript type system then we’re making type-level promises which we cannot actually guarantee.

[JordanMartinez]

Sounds good.

[jamesdbrock]

Can you clarify why the Node.HTTP2.Internal module exists?

It seems like you write FFI once that reuse it in both the client and server files.

Yes.

Was that to reduce the amount of code that needed to be written? Or is there some other reason?

Mostly because I think this way presents the nicest and simplest API to the user.

If the HttpServer' connectionType design is utilized and that design is used in other places (e.g. ClientHttp2Session/ServerHttp2Session -> Http2Session clientOrServerType), would the module still be needed? Or put differently, could the module instead be called Shared.purs which indicates its functions can be used by either the client or server?

We could do it that way, that would be a reasonable approach. But in my opinion making that parameterized Http2Session' type is clutter which doesn’t clarify things for the user at all. There are only two possible parameters for the type, so there are only two possible types. It’s better for the API to present just the two types

• ServerHttp2Session :: Type
• ClientHttp2Session :: Type

than to present this little type substitution puzzle to the user and force them to mentally figure out that there are only two solutions.

• Server :: Type
• Client :: Type
• Http2Session' :: Type -> Type

And, furthermore, if the user already has Node.js experience then it's easy for them to recognize type names from Node.js. Because they are exactly the same.

Node.js HTTP/2	PureScript Node.HTTP2
Http2Server	Http2Server
Http2SecureServer	Http2SecureServer
ServerHttp2Session	ServerHttp2Session
ClientHttp2Session	ClientHttp2Session
ServerHttp2Stream	ServerHttp2Stream
ClientHttp2Stream	ClientHttp2Stream

So then after we've made this API choice, we're left with the question of how to implement it simply. I decided the simplest thing to do is to implement the FFI functions once and then unsafeCoerce their types. So at that point internally we don’t get any help from the PureScript typechecker. But the API is type-safe.

P.S. I recently de-parameterized the int64 package in a similar way. I changed the API from Long Signed and Long Unsigned into Int64 and UInt64 and IMHO it’s a big improvement.

[jamesdbrock]

I think the FFI should be using types like EffectFnX instead of the curried variants. The code will optimized better with purs-backend-es that way (either now or in the future).

Yeah that would be better.

But it it doesn't change the API at all, so it could be done in a later PR with a patch version bump. And it won't make much difference to performance for this package, because the expensive part is the Aff binds and syscalls to network I/O, not the function calls.

[jamesdbrock]

The worst problem with this PR is that you can't use this code to write a webserver that serves both HTTP/1 and HTTP/2 on the same port; see the PR description note. That seems to me like a catastrophic problem and solving it would require deleting everything and rewriting against a completely different Node.js HTTP/2 API. I didn't notice this was true until I had basically finished and was writing corner-case tests.

[JordanMartinez]

But in my opinion making that parameterized Http2Session' type is clutter which doesn’t clarify things for the user at all. There are only two possible parameters for the type, so there are only two possible types. It’s better for the API to present just the two types

I can agree with the above statement (that simpler types makes it easier for the developer) by disagreeing with the below comment:

I decided the simplest thing to do is to implement the FFI functions once and then unsafeCoerce their types. So at that point internally we don’t get any help from the PureScript typechecker. But the API is type-safe.

I think we should minimize unsafeCoerce usage in this case by just duplicating the FFI. Whenever there are bugs in PureScript code, it's usually caused by the FFI being wrong. If there is a way to write the FFI without unsafeCoerce, then it should be done without that.

However, I'm still not sure this is worth doing. Other libraries (e.g. node-streams) does use the type system to define a single API that can be used on two different types (e.g. readable streams and duplex streams can both use onData events). So, while it may make things easier here, is it going to matter?
Also, if I was a new user to PureScript, I would appreciate this. But once I better understood the language, I wonder if I would still appreciate it because now I need to remember which of the two modules to use to get the function with the correct type signature.

[jamesdbrock]

I think we should minimize unsafeCoerce usage

Whenever there are bugs in PureScript code, it's usually caused by the FFI being wrong. If there is a way to write the FFI without unsafeCoerce, then it should be done without that.

I agree with your characterization of the trade-offs here.

I thought about it a bit and realized that we can get a lot more type-safety at the FFI by adding only a little bit more code, while keeping the same public API.

I just added a commit which restricts the use of unsafeCoerce only to up-casting a single JavaScript object to its superclass. Do you like this? I like it.

(I also found and fixed one type error while doing this)

[JordanMartinez]

I think we should minimize unsafeCoerce usage

Whenever there are bugs in PureScript code, it's usually caused by the FFI being wrong. If there is a way to write the FFI without unsafeCoerce, then it should be done without that.

I agree with your characterization of the trade-offs here.

I thought about it a bit and realized that we can get a lot more type-safety at the FFI by adding only a little bit more code, while keeping the same public API.

I just added a commit which restricts the use of unsafeCoerce only to up-casting a single JavaScript object to its superclass. Do you like this? I like it.

I think we disagree philosophically about how FFI should be written 😄 When I said "If there is a way to write the FFI without unsafeCoerce, then it should be done without that.", I really mean it. unsafeCoerce should only be done when it's necessary and it seems to me that it's not in this situation (unlike the Child Process PR where passing a String, Stream, or undefined is necessary).

[JordanMartinez]

Regardless, I think we should ignore the FFI part of this PR for now. I should review the actual bindings themselves. I think I initially brought up the way the FFI was done because it made reviewing the bindings harder for me to follow. All in all, I just need to get more familiar with Node's http2 module.

This PR does not support a listening server that can handle both HTTP/2 and HTTP/1.x.

This may seem like a dumb question, but why is that bad? How often is HTTP/1.x even used nowadays? Is there some other setup one could do (e.g. an HTTP/1.x server that just redirects to the HTTP/2 one)?

[jamesdbrock]

I think we disagree philosophically about how FFI should be written smile

Yeah 😄

When I said "If there is a way to write the FFI without unsafeCoerce, then it should be done without that.", I really mean it. unsafeCoerce should only be done when it's necessary and it seems to me that it's not in this situation

Okay here's a thing we could do. We could use the PureScript type system to explictly declare which typecasts are legal. Apparently I'm not the first to consider this because this OCaml enthusiast has already implemented exactly what I had in mind:

https://pursuit.purescript.org/packages/purescript-cast/0.1.0/docs/Data.Cast

Then we can replace every remaining unsafeCoerce with cast.

EDIT

Or, I mean, we could do this without any type classes and just have a bunch of internal upcasting functions like:

upcastClientHttp2Session :: ClientHttp2Session -> Http2Session
upcastClientHttp2Session = unsafeCoerce

I'm going to do that and push another commit.

EDIT

I pushed a commit to type-annotate all unsafeCoerce Javascript object casts.

[jamesdbrock]

This PR does not support a listening server that can handle both HTTP/2 and HTTP/1.x.

This may seem like a dumb question, but why is that bad? How often is HTTP/1.x even used nowadays?

I suppose it’s bad if one wants write a webserver for the public internet? I don't know how much public internet traffic is HTTP/1-only nowadays.

Is there some other setup one could do (e.g. an HTTP/1.x server that just redirects to the HTTP/2 one)?

Maybe, I don't know. I would have to research and test that.

[JordanMartinez]

• Could you clarify why bindings for the following events weren't implemented?
  • HttpSession.on("close") - even though httpSession.close registers the provided callback on the close events, excluding this binding prevents someone from registering multiple callbacks. In a another case, the server can call close but the client can't run any code once it receives the close event.
  • HttpSession.on("connect")
  • HttpSession.on("error") - seems the server has this but not the client?
  • HttpSession.on("frameError")
  • HttpSession.on("goaway")
  • HttpSession.on("localSettings")
  • HttpSession.on("ping")
  • HttpSession.on("remoteSettings")
  • HttpSession.on("stream") - server has this but not the client?
  • HttpSession.on("timeout")

....

I spent a bit of time looking through the Node docs and this PR. Emotionally, I'm.... irritated 😅. I was (probably unreasonably) expecting all the bindings from http2 to be implemented and ordered in a way that's similar to how the Node docs present them, so I could verify from top-to-bottom, one-by-one that each had been done correctly. I then found that many events for HttpSession weren't implemented. And in the case of the close event, it seems it was not implemented because httpSession.close(cb) can be used to add the cb as an event listener to the close event. But it took me a while to realize that. And even after understanding that, the missing binding for the corresponding event seems problematic.

So, it's hard for me to review this PR for a few reasons:

• My previous disagreement with the FFI approach taken cost me some motivation because in the back of my mind, the perfectionist inside of me is distracting me with its yelling.
• While I can review just the bindings currently implemented, I wonder if there may be "gotchas" to not reading the rest of the docs in Node. So, I can't just approve a binding based on the Node docs for that binding alone. Sometimes, there are Node docs in another spot that clarifies things for this spot (something I've learned while implementing bindings to child_process). It feels like a waste to read through all the Node docs just to approve a select few bindings that seem specific to your use case.
• The more irritated/emotional I become with this current PR, the more I feel like just implementing these bindings myself because then I can theoretically resolve all of my problems.

[JordanMartinez]

I talked with some coworkers about this PR and want to propose how to deal with the current situation.

At the end of the day, you have implemented some bindings. I'm sure they work because I know you're competent, and that if you came across some issue with them, you would fix it. Even though these bindings don't fully support the entirety of the http2 module, these bindings are still useful and a net-gain for the community as a whole. If anything, I am likely discouraging you from contributing in the future due to my ideology/principles. If I have issues about how the FFI is implemented here, there are or have been other libraries (e.g. node-fs) that did/have done worse. I was reminded that, "at the end of the day, all FFI is unsafeCoerce". So whatever my ideology, I believe I'm fighting the wrong battle in this situation.

Moreover, it was pointed out to me that the node libraries are designed to provide bindings specific to each node module. This makes it easier to know which library to use to get access to various bindings. When you asked whether these bindings should be in this library or a new one, I said to stick them here. This goes against the node library conventions/philosophy as to where things are stored. I was being undisciplined before. Now, I think the http2 bindings should be in their own library.

Anyway, here's what I'd like to propose. I propose we add a new repo for node-http2, make you the main maintainer for that repo, and push this PR's http2 code there. That removes the problem of me slowing this down because of my ideology. It grants you the freedom to add more bindings as needed. It gives the community a central library for http2 bindings.
I further propose that this PR be reduced to just fixing the issues specific to this library.

How does that sound?

[jamesdbrock]

I think the http2 bindings should be in their own library. How does that sound?

Fine. I'm ambivalent about this PR. When I started I was hoping that I would be able to understand the essential Node.js HTTP/2 API and that there would be one correct way to express it in Aff. But things didn’t work out that way and I had to make choices and compromises. I think these are good choices but a reasonable person could make different ones. And you have your objections, so we can agree this is probably best as a separate package.

Thanks for spending time reviewing this PR @JordanMartinez , you pointed out several areas for improvement in the future.

[jamesdbrock]

(Note to myself or anyone else who wants to revive this: After I closed the Github PR I removed the third commit and force-pushed the http2 branch, which appears to have borked the Github PR. Anyway, the code I recommend using is the first two commits on the http2 branch.)

[klarkc]

Anyway, here's what I'd like to propose. I propose we add a new repo for node-http2, make you the main maintainer for that repo, and push this PR's http2 code there.

@JordanMartinez Just to clarify, is the node-http2 repo the one in question? Should we move issue #44 to this repo? Is this code already there?

[JordanMartinez]

Yes, node-http2 is the bindings to the http2 module. Note: it depends on some breaking changes made throughout the Node libraries. So, some libraries haven't been updated to use the breaking changes yet.

[jamesdbrock]

Excerpt from the docs for this PR. This is what the API basically looks like.

Server-side example

Equivalent to Node.js HTTP/2 Core API Server-side example

import Node.Stream.Aff (write, end)

key <- Node.FS.Sync.readFile "localhost-privkey.pem"
cert <- Node.FS.Sync.readFile "localhost-cert.pem"

either (liftEffect <<< Console.errorShow) pure =<< attempt do
  server <- createSecureServer (toOptions {key, cert})
  listenSecure server (toOptions {port:8443})
    \session headers stream -> do
      respond stream
        (toOptions {})
        (toHeaders
          { "content-type": "text/html; charset=utf-8"
          , ":status": 200
          }
        )
      write (toDuplex stream) =<< fromStringUTF8 ("<h1>Hello World<hl>")
      end (toDuplex stream)

Client-side example

Equivalent to Node.js HTTP/2 Core API Client-side example

import Node.Stream.Aff (readAll)

ca <- liftEffect $ Node.FS.Sync.readFile "localhost-cert.pem"

either (liftEffect <<< Console.errorShow) pure =<< attempt do
  client <- connect
    (toOptions {ca})
    (URL.parse "https://localhost:8443")

  stream <- request client
    (toOptions {})
    (toHeaders {":path": "/"})

  headers <- waitResponse stream
  liftEffect $ for_ (headerKeys headers) \name ->
    Console.log $
      name <> ": " <> fromMaybe "" (headerString headers name)

  body <- toStringUTF8 =<< (fst <$> readAll (toDuplex stream))
  liftEffect $ Console.log $ "\n" <> body

  closeSession client