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

feat(http-server): add http/2 support based on spdy #2065

Closed
wants to merge 1 commit into from
Closed

Conversation

raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented Nov 21, 2018

Adds http/2 support using https://github.com/spdy-http2/node-spdy. Even Node.js core now has http2 support, the spdy module is express-compatible.

Checklist

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

@bajtos
Copy link
Member

bajtos commented Nov 23, 2018

+1 for adding support for HTTP/2.

Personally, I'd prefer to use the built-in HTTP/2 implementation provided by Node.js core, I expect it will have better integration with tooling like traces, monitoring, debugging, etc. and also get security issues fixed more quickly.

The built-in http2 module is marked as Stable in all Node.js releases we support (8.x, 10.x), see the docs.

@raymondfeng What are your arguments for using a user-land module instead?

@bajtos
Copy link
Member

bajtos commented Nov 23, 2018

Also note that the built-in http2 modules supports HTTP/2 over uncencrypted connection too, which is useful for local testing.

@bajtos
Copy link
Member

bajtos commented Nov 23, 2018

See also http2's Compatibility API

@@ -84,14 +91,22 @@ export class HttpServer {
*/
constructor(
requestListener: RequestListener,
serverOptions?: HttpServerOptions,
serverOptions: HttpServerOptions = {},
) {
this.requestListener = requestListener;
this.serverOptions = serverOptions;
this._port = serverOptions ? serverOptions.port || 0 : 0;
this._host = serverOptions ? serverOptions.host : undefined;
this._protocol = serverOptions ? serverOptions.protocol || 'http' : 'http';
Copy link
Member

Choose a reason for hiding this comment

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

Since serverOptions are always defined, the checks serverOptions ?: can be removed now. (This applies to all three lines above.)

@bajtos
Copy link
Member

bajtos commented Nov 23, 2018

@raymondfeng I'd like to better understand the motivations behind this change and the goals you have in mind.

  • What are the scenarios you would like to enable by this pull request?
  • HTTP/2 has a different programming offering a richer set of features compared to HTTP/1. What subset of these features are trying to provide by this pull request? I'd like us to be explicit about which parts of HTTP/2 we do and do not support.
  • What is the priority of this work? Who is waiting for this feature?

In general, I am very reluctant to force node-spdy dependency on all LB4 users.

Also, because HttpServer is exposing the underlying HTTP/1 or HTTP/2 server as a public API, we must carefully consider ramifications of committing to node-spdy server API (as opposed to core http2 APIs).

From what I was able to find about http2 and express integration, express@4 is not compatible with http2 because it manipulates internals of the built-in request/response objects in an unsupported way. The upcoming express@5 version is aiming to fix that problem (see expressjs/express#3730, and also an older expressjs/express#2761 (comment)), unfortunately the release is probably months to years in the future.

On the other hand, from what I remember, we use only a small subset of Express functionality. I believe we are relying on two features only:

  • enhanced request/response objects (as expected by 3rd-party middleware handlers)
  • a basic router capable of executing middleware handlers in the order they were registered

If that's the case, then I am proposing to rework our internal implementation, drop full Express and replace it with our own composition of lower-level Express components (see https://github.com/pillarjs) from the upcoming version express@5. That way we can stay Express compatible and be able to leverage native http2 module.

Or perhaps even better, replace Express with Fastify which claims compatibility with Express middleware and native HTTP/2 too.

Let's discuss!

@bajtos
Copy link
Member

bajtos commented Nov 23, 2018

HTTP/2 has a different programming offering a richer set of features compared to HTTP/1. What subset of these features are trying to provide by this pull request? I'd like us to be explicit about which parts of HTTP/2 we do and do not support.

Besides the obvious push-based approach for serving files, do we want to support ALPN-based support for falling back to HTTP/1.1? See https://nodejs.org/api/http2.html#http2_alpn_negotiation

@bajtos
Copy link
Member

bajtos commented Nov 23, 2018

@raymondfeng if your goal is to quickly add an experimental support for HTTP/2 based on https://github.com/spdy-http2/node-spdy, then I am proposing to implement it as a new extension and modify @loopback/rest to allow app/extensions to provide a custom server factory (provider).

I am thinking about changing the following line:

https://github.com/strongloop/loopback-next/blob/b9570c8f9e6b5b9e57271605e8e132b5815a2198/packages/rest/src/rest.server.ts#L742

into something like this:

this._httpServer = this.httpServerFactory(this.requestHandler, serverOptions);

By default, httpServerFactory can be implemented via new HttpServer(), but users are allowed to provide a custom factory function via Dependency Injection.

app.bind(RestBindings.SERVER_FACTORY).to((handler, options) => {
  return new SpdyServer(
    options as spdy.ServerOptions,
    handler,
  );
});

class SpdyServer implements HttpServer {
 // use spdy.createServer() under the hood
 // provide HttpServer features like port/host/url, start & stop, etc.
}

Just an idea to kickstart the discussion.

@raymondfeng
Copy link
Contributor Author

I'm exploring the opportunity to leverage HTTP/2 + SSE to support #1884. At this moment, I'm mostly trying to use http/2 as a transparent drop-in enhancement to http/https without touching the http/2 specific programming model level features yet.

The primary motivtion to use spdy is its express compatibility.

@bajtos
Copy link
Member

bajtos commented Nov 26, 2018

I'm exploring the opportunity to leverage HTTP/2 + SSE to support #1884. At this moment, I'm mostly trying to use http/2 as a transparent drop-in enhancement to http/https without touching the http/2 specific programming model level features yet.

Makes sense 👍

I believe HTTP/2 is not required for SSE (server-sent events), SSE works with HTTP/1 too. For example, LB3 (strong-remoting) is using SSE to stream notification about model updates - see http-context.js for the implementation details, the docs and the example client.

Also as far as I understand HTTP/2, it does not really provide any benefits for server-sent events compared to HTTP/1. It's always the client who has to initiate the request. The server can speculatively send content for a request that will be requested soon (e.g. css/js assets for a HTML page), but it's still up to the client to request that content. I.e. there is no way for the HTTP/2 server to actively push a new event (a notification) to the client, SSE is using the same never-ending stream as HTTP/1.

I am trying to say that missing support for HTTP/2 should not be a blocker for your work on #1884.

The primary motivtion to use spdy is its express compatibility.

I understand that very well after my short research on express+HTTP/2 compatibility on GitHub. What a mess :(

I see @loopback/rest as a core component of LB4, one that will be used by most LB4 apps. I am very reluctant to force a new dependency on node-spdy to all LB4 users that are happy with HTTP/1 and don't ask for HTTP/2. I strongly believe that a built-in support for HTTP/2 should be implemented on top of Node.js core module http2.

At the same time, I am perfectly fine to provide HTTP/2 support based on node-spdy via a new extension package. Because this package is versioned independently and used by a small fraction of our user base, we have more space for iterating and learning from mistakes.

Also by adding an extension point allowing applications/extensions to contribute custom HttpServer implementation, we make LoopBack even more future proof. When HTTP/3 comes closer, we can create another extension to run LoopBack on HTTP/3, even before there is a rock-solid HTTP/3 implementation available for general use.

@raymondfeng
Copy link
Contributor Author

It's not a blocker to support SSE with HTTP 1.1. But HTTP/2 + SSE is much more compelling.

It's fair to have an extension for http2. I'll see what I can do.

@raymondfeng
Copy link
Contributor Author

Superseded by #2078

@bajtos bajtos deleted the spdy branch November 30, 2018 07:35
@bajtos
Copy link
Member

bajtos commented Nov 30, 2018

@raymondfeng FYI, a Node.js core maintainer is warning against using node-spdy.

https://twitter.com/addaleax/status/1068063898228727814

Please do not use the spdy module on npm, if at all possible.

Its code depends on Node's internals in an incredibly fragile way, and hasn't worked on Node 10 for most of its lifetime.

SPDY itself is deprecated in favour of HTTP/2; modern browsers have dropped support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants