feat(http-server): make http2 server creation extensible #2078

raymondfeng · 2018-11-26T19:20:38Z

Supercedes #2065

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 · 2018-11-27T14:12:04Z

packages/http-server/src/http-server.ts


 /**
 * HTTP / HTTPS server used by LoopBack's RestServer
 *
 * @export
 * @class HttpServer
 */
-export class HttpServer {
+export class DefaultHttpServer implements HttpServer {


What's the impact of this change on possible users of @loopback/http-server?

For example, the following code creates a fully working HttpServer class in the current version (before your changes):

import * as turbo from 'turbo-http'; class TurboHttpServer extends HttpServer { constructor( requestListener: RequestListener, serverOptions?: HttpServerOptions, ) { super(requestListener, serverOptions); this.server = turbo.createServer(requestListener); } }

IMO, we should preserve backwards compatibility in our API. Otherwise a new major version must be published, plus we will have to keep maintaining the old version according to our LTS policy...

We can release @loopback/[email protected] if we have to break the compatibility due to the nature of this work.

We can release @loopback/[email protected] if we have to break the compatibility due to the nature of this work.

Fair enough. In that case:

The commit message in this PR should clearly describe the breaking changes so that our release tooling can pick it up. Ideally, there should be a short migration guide too.

We need to update http-server's README with a table listing supported major versions (LTS terms). See https://github.com/strongloop/loopback#module-long-term-support-policy for inspiration. This should be done before 2.0.0 release, so that the new content is displayed on https://www.npmjs.com/package/@loopback/http-server

See also #2103

bajtos · 2018-11-27T14:13:57Z

packages/http-server/src/types.ts

+ * @interface Http2Options
+ */
+export interface Http2Options extends ListenerOptions {
+  protocol: 'http2';


The built-in http2 module support HTTP/2 protocol over plain unencrypted socket (similar to HTTP) or over TLS socket (similar to HTTPS). How do you envision encoding this configuration flag when there is a single protocol value http2?

bajtos · 2018-11-27T14:16:09Z

packages/http-server/test/integration/http-server.integration.ts

    await server.start();
    expect(server.url).to.equal(`http://127.0.0.1:${server.port}`);
  });

+  it('supports HTTP/2 protocol with key and certificate files', async () => {
+    // For some reason, spdy fails with Node 11
+    if (+process.versions.node.split('.')[0] > 10) return;


This early return will mark the test as passed, leading to false sense of security.

Please mark the test as skipped on Node.js 11.x. If we don't have a helper like "it.skipIf", then please consider creating one in @loopback/testlab.

it.skipIf( // For some reason, spdy fails with Node 11 +process.versions.node.split('.')[0] > 10), 'supports HTTP/2 protocol with key and certificate files', async () => { // ... } );

bajtos · 2018-11-27T14:16:38Z

packages/http-server/test/integration/http-server.integration.ts

+    if (+process.versions.node.split('.')[0] > 10) return;
+    const serverOptions: Http2Options = Object.assign(
+      {
+        protocol: 'http2' as 'http2',


'http2' as 'http2'

This looks weird, are you sure it's needed?

If we don't cast 'http2' to 'http2', TS complains as it infers the type of http2 as string, which is not compatible with 'http' | 'https' | 'http2'.

Weird.

Can you add a comment to explain to readers please? (Similarly for 'h2' below.)

bajtos · 2018-11-27T14:16:54Z

packages/http-server/test/integration/http-server.integration.ts

+      {
+        protocol: 'http2' as 'http2',
+        rejectUnauthorized: false,
+        spdy: {protocols: ['h2' as 'h2']},


similarly here: 'h2' as 'h2'?

bajtos · 2018-11-27T14:18:07Z

packages/http-server/test/integration/http-server.integration.ts

+    // We need to close the agent so that server.close() returns
+    // `@types/[email protected]` is not fully compatible with `[email protected]`
+    // tslint:disable-next-line:no-any
+    (agent as any).close();


Please close the agent before making assertion.

In your current version, when expect(response.statusCode) throw an assertion error, then the agent is never closed.

packages/http-server/src/http-server.ts

bajtos · 2018-11-27T14:20:39Z

packages/http-server/src/types.ts

+/**
+ * A factory to create http servers
+ */
+export interface HttpServerFactory {


Can we use a plain function instead of an interface please?

I wanted to make it possible to use dependency injection for the implementation.

Makes sense. Can you enhance the TSDoc comment to explain why we choose an interface instead of a function?

bajtos · 2018-11-27T14:22:51Z

packages/testlab/src/request.ts

+  agent?: https.Agent,
+): Promise<IncomingMessage> {
+  agent =
+    agent ||


I find the formatting of this code block rather weird. Have you considered the following alternative?

const defaultAgent = new https.Agent({ rejectUnauthorized: false, }); export function httpsGetAsync( urlString: string, agent: https.Agent = defaultAgent, ): Promise<IncomingMessage> { // agent is always set now }

bajtos · 2018-11-30T08:29:29Z

@raymondfeng I am not very happy with the high-level design. Let's discuss and see if we can improve it.

I find unusual that we are injecting a factory. From what I've seen in applications using Dependency Injection, object creation (class instantiation) is usually handled by the IoC framework and the code receives the final object, not a factory to create it.

Can we change the design so that the HTTP Server implementation (e.g spdy) is configured by binding the server class, leaving the factory part for Context?

app.bind(RestBindings.Http.SERVER).toClass(Http2Server)

A typical HttpServer has two constructor-level dependencies: request-handler function and configuration object.

Request-handler function is already bound to the context, see https://github.com/strongloop/loopback-next/blob/b9cb3e0689de93a4aa76414265f0c4050bfd0e17/packages/rest/src/rest.server.ts#L196
Configuration is partially bound in the context too, see https://github.com/strongloop/loopback-next/blob/b9cb3e0689de93a4aa76414265f0c4050bfd0e17/packages/rest/src/rest.server.ts#L185-L188 It should be enough to create a new binding to hold all configuration options in a single object.

I think this change will greatly simplify both our implementation and also extensions providing custom servers.

bajtos · 2018-11-30T08:30:46Z

One more reason why I don't like HttpServerFactory is that HttpServer itself is implementing as a factory that accepts protocol and creates a different underlying server instance based on the configured value (http.createServer, https.createServer, etc.)

bajtos · 2018-12-06T14:44:53Z

packages/http-server/src/types.ts

+}
+
+export interface ProtocolServerFactory {
+  supports(protocol: string, serverOptions: HttpServerOptions): boolean;


Interesting. The good part about this design is that it follows what we have already in place for body parsers. However, I feel it's unnecessarily complicated for the case of http servers.

For long term, I think we need to provide http2 support based on Node.js core module http2. From what I read, spdy does not work on Node.js 10+. Once this is done, most users should not need to change the way how HTTP servers are created at all.

raeef-refai · 2019-01-28T12:50:54Z

@raymondfeng @bajtos any news about this feature?

stale · 2020-01-24T03:29:01Z

This issue has been marked stale because it has not seen activity within six months. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository. This issue will be closed within 30 days of being stale.

stale · 2020-02-23T05:55:09Z

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.

vaibhavkumar-sf · 2022-07-01T08:23:42Z

Hey, we need http2 support in LB4,

raymondfeng requested review from bajtos and hacksparrow as code owners November 26, 2018 19:20

raymondfeng mentioned this pull request Nov 26, 2018

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

Closed

7 tasks

bajtos reviewed Nov 27, 2018

View reviewed changes

bajtos mentioned this pull request Nov 30, 2018

docs: describe the process for making breaking changes #2103

Merged

3 tasks

feat(http-server): make http2 server creation extensible

4d8c374

raymondfeng force-pushed the extend-http-server branch from b9cb3e0 to 4d8c374 Compare November 30, 2018 23:43

bajtos reviewed Dec 6, 2018

View reviewed changes

bajtos added feature REST Issues related to @loopback/rest package and REST transport in general labels Jan 7, 2019

stale bot added the stale label Jan 24, 2020

stale bot closed this Feb 23, 2020

vaibhavkumar-sf mentioned this pull request Jul 1, 2022

feat(http-server): adds http/2 support #4989

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(http-server): make http2 server creation extensible #2078

feat(http-server): make http2 server creation extensible #2078

raymondfeng commented Nov 26, 2018 •

edited

Loading

bajtos Nov 27, 2018

raymondfeng Nov 27, 2018

bajtos Nov 30, 2018

bajtos Nov 30, 2018

bajtos Nov 27, 2018

bajtos Nov 27, 2018

bajtos Nov 27, 2018

raymondfeng Nov 27, 2018

bajtos Nov 30, 2018

bajtos Nov 27, 2018

bajtos Nov 27, 2018

bajtos Nov 27, 2018

raymondfeng Nov 27, 2018

bajtos Nov 30, 2018

bajtos Nov 27, 2018

raymondfeng Nov 27, 2018

bajtos commented Nov 30, 2018 •

edited

Loading

bajtos commented Nov 30, 2018

bajtos Dec 6, 2018

raeef-refai commented Jan 28, 2019 •

edited

Loading

stale bot commented Jan 24, 2020

stale bot commented Feb 23, 2020

vaibhavkumar-sf commented Jul 1, 2022

feat(http-server): make http2 server creation extensible #2078

feat(http-server): make http2 server creation extensible #2078

Conversation

raymondfeng commented Nov 26, 2018 • edited Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Nov 30, 2018 • edited Loading

bajtos commented Nov 30, 2018

Choose a reason for hiding this comment

raeef-refai commented Jan 28, 2019 • edited Loading

stale bot commented Jan 24, 2020

stale bot commented Feb 23, 2020

vaibhavkumar-sf commented Jul 1, 2022

raymondfeng commented Nov 26, 2018 •

edited

Loading

bajtos commented Nov 30, 2018 •

edited

Loading

raeef-refai commented Jan 28, 2019 •

edited

Loading