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

Implement HTTP endpoint factory #1369

Merged
merged 1 commit into from
Jun 8, 2018
Merged
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
1 change: 1 addition & 0 deletions packages/http-server/.npmrc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
package-lock=false
57 changes: 57 additions & 0 deletions packages/http-server/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
# @loopback/http-server

This package implements the HTTP / HTTPS server endpoint for LoopBack 4 apps.

## Overview

This is an internal package used by LoopBack 4 for creating HTTP / HTTPS server.

## Installation

To use this package, you'll need to install `@loopback/http-server`.

```sh
npm i @loopback/http-server
```

## Usage

`@loopback/http-server` should be instantiated with a request handler function, and an HTTP / HTTPS options object.

```js
const httpServer = new HttpServer((req, res) => { res.end('Hello world')}, {port: 3000, host: ''});
```

Instance methods of `HttpServer`.

| Method | Description |
| ------- | -------------------- |
| `start()` | Starts the server |
| `stop()` | Stops the server |

Instance properties of `HttpServer`.

| Property | Description |
| ----------- | ---------------------- |
| `address` | Address details |
| `host` | host of the server |
| `port` | port of the server |
| `protocol` | protocol of the server |
| `url` | url the server |

## Contributions

- [Guidelines](https://github.com/strongloop/loopback-next/wiki/Contributing#guidelines)
- [Join the team](https://github.com/strongloop/loopback-next/issues/110)

## Tests

Run `npm test` from the root folder.

## Contributors

See [all contributors](https://github.com/strongloop/loopback-next/graphs/contributors).

## License

MIT
1 change: 1 addition & 0 deletions packages/http-server/index.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from './dist8';
6 changes: 6 additions & 0 deletions packages/http-server/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// Copyright IBM Corp. 2017. All Rights Reserved.
// Node module: @loopback/http-server
// This file is licensed under the MIT License.
// License text available at https://opensource.org/licenses/MIT

module.exports = require('@loopback/dist-util').loadDist(__dirname);
6 changes: 6 additions & 0 deletions packages/http-server/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// Copyright IBM Corp. 2017,2018. All Rights Reserved.
// Node module: @loopback/http-server
// This file is licensed under the MIT License.
// License text available at https://opensource.org/licenses/MIT

export * from './src';
50 changes: 50 additions & 0 deletions packages/http-server/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
{
"name": "@loopback/http-server",
"version": "0.0.1",
"description": "",
"engines": {
"node": ">=8"
},
"scripts": {
"acceptance": "lb-mocha \"DIST/test/acceptance/**/*.js\"",
"build": "npm run build:dist8 && npm run build:dist10",
"build:apidocs": "lb-apidocs",
"build:current": "lb-tsc",
"build:dist8": "lb-tsc es2017",
"build:dist10": "lb-tsc es2018",
"clean": "lb-clean loopback-http-server*.tgz dist* package api-docs",
"pretest": "npm run build:current",
"integration": "lb-mocha \"DIST/test/integration/**/*.js\"",
"test": "lb-mocha \"DIST/test/unit/**/*.js\" \"DIST/test/integration/**/*.js\" \"DIST/test/acceptance/**/*.js\"",
"unit": "lb-mocha \"DIST/test/unit/**/*.js\"",
"verify": "npm pack && tar xf loopback-http-server*.tgz && tree package && npm run clean"
},
"author": "IBM",
"copyright.owner": "IBM Corp.",
"license": "MIT",
"dependencies": {
"@loopback/dist-util": "^0.3.1",
"p-event": "^2.0.0"
},
"devDependencies": {
"@loopback/build": "^0.6.5",
"@loopback/core": "^0.8.4",
"@loopback/testlab": "^0.10.4",
"@types/node": "^10.1.2",
"@types/p-event": "^1.3.0",
"@types/request-promise-native": "^1.0.14",
"request-promise-native": "^1.0.5"
},
"files": [
"README.md",
"index.js",
"index.d.ts",
"dist*/src",
"dist*/index*",
"src"
],
"repository": {
"type": "git",
"url": "https://github.com/strongloop/loopback-next.git"
}
}
119 changes: 119 additions & 0 deletions packages/http-server/src/http-server.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
// Copyright IBM Corp. 2017,2018. All Rights Reserved.
// Node module: @loopback/http-server
// This file is licensed under the MIT License.
// License text available at https://opensource.org/licenses/MIT

import {createServer, Server, ServerRequest, ServerResponse} from 'http';
import {AddressInfo} from 'net';
import * as pEvent from 'p-event';

export type HttpRequestListener = (
req: ServerRequest,
res: ServerResponse,
) => void;

/**
* Object for specifyig the HTTP / HTTPS server options
*/
export type HttpServerOptions = {
port?: number;
host?: string;
protocol?: HttpProtocol;
};

export type HttpProtocol = 'http' | 'https'; // Will be extended to `http2` in the future

/**
* HTTP / HTTPS server used by LoopBack's RestServer
*
* @export
* @class HttpServer
*/
export class HttpServer {
private _port: number;
private _host?: string;
private _started: Boolean;
private _protocol: HttpProtocol;
private _address: AddressInfo;
private httpRequestListener: HttpRequestListener;
private httpServer: Server;

/**
* @param httpServerOptions
* @param httpRequestListener
*/
constructor(
httpRequestListener: HttpRequestListener,
httpServerOptions?: HttpServerOptions,
) {
this.httpRequestListener = httpRequestListener;
if (!httpServerOptions) httpServerOptions = {};
this._port = httpServerOptions.port || 0;
this._host = httpServerOptions.host || undefined;
this._protocol = httpServerOptions.protocol || 'http';
}

/**
* Starts the HTTP / HTTPS server
*/
public async start() {
this.httpServer = createServer(this.httpRequestListener);
this.httpServer.listen(this._port, this._host);
await pEvent(this.httpServer, 'listening');
Copy link
Contributor

@raymondfeng raymondfeng Jun 6, 2018

Choose a reason for hiding this comment

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

I wonder why we don't use a callback for httpServer.listen() and close(). For example:

import {promisify} from 'util';

public async start() {
  this.httpServer = createServer(this.httpRequestListener);
  await promisify(this.httpServer.listen)(this._port, this._host);
  this._address = this.httpServer.address() as AddressInfo;
  this._host = this._host || this._address.address;
  this._port = this._address.port;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, will add.

Copy link
Member

Choose a reason for hiding this comment

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

Please note that pEvent will listen for error events too. As a result, server.start() returns a rejected promise if the server cannot be started.

The callback for httpServer.listen is not error-first, it's just a regular event callback for listening event. See https://nodejs.org/dist/latest-v10.x/docs/api/net.html#net_server_listen:

When the server starts listening, the 'listening' event will be emitted. The last parameter callback will be added as a listener for the 'listening' event.

@hacksparrow Please start by adding a test to verify that start() returns a rejected promise when the server cannot be started. Quoting from the same doc page:

One of the most common errors raised when listening is EADDRINUSE. This happens when another server is already listening on the requested port / path / handle.

In the test:

  • create and start a server listening on port 0 (let the OS pick an available port)
  • create another server, set the port number explicitly to the same value as reported by the server started in the previous step
  • verify that start() returns a rejected promise and the error has code set to EADDRINUSE - see http://shouldjs.github.io/#assertion-rejectedwith

Once we have that test in place, you can try to use the promisifed listen method and see how easy/difficult it is to correctly handle errors too (compared to what we have now).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bajtos added the test.

@raymondfeng why have callbacks, when we can just await?

this._started = true;
this._address = this.httpServer.address() as AddressInfo;
}

/**
* Stops the HTTP / HTTPS server
*/
public async stop() {
if (this.httpServer) {
this.httpServer.close();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we expose the state of the HttpServer, such as started: boolean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion, will add. Initially I had listening, but removed it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

await pEvent(this.httpServer, 'close');
this._started = false;
}
}

/**
* Protocol of the HTTP / HTTPS server
*/
public get protocol(): HttpProtocol {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why bother to have getter for these properties? Do you want to enforce readonly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, readonly. What do you suggest?

Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tried readonly keyword from TypeScript?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, will use that.

Copy link
Member

Choose a reason for hiding this comment

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

I think we can use readonly modifier for all properties excluding port, because port changes as a result of start() call. OTOH, maybe we should keep a readonly _port property to hold the requested port (e.g. 0) and then write a port getter that will either return the actual port reported by this._httpServer.address().port or fall back to this._port.

This has an important benefit that multiple start/stop cycles of a server configured to listen on port 0 will always pick a new port. At the moment, I believe subsequent start() calls will pick the temporary port number assigned by the operating system for the first start() call.

@hacksparrow Please add a unit-test to verify the behavior of our server in this edge case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@raymondfeng even an undefined host will behave like port set to 0. If the host value is not set during instantiation, the HttpServer instance will store it as undefined, however, the underlying http/https will generate a host value for the server - something like ::.

Since url is dependent on host and port, the only property that can be implemented as a readonly would be protocol.

@bajtos I will add tests for both host and port.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

return this._protocol;
}

/**
* Port number of the HTTP / HTTPS server
*/
public get port(): number {
return (this._address && this._address.port) || this._port;
}

/**
* Host of the HTTP / HTTPS server
*/
public get host(): string | undefined {
return (this._address && this._address.address) || this._host;
}

/**
* URL of the HTTP / HTTPS server
*/
public get url(): string {
return `${this._protocol}://${this.host}:${this.port}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

+1 to handle IPv6 localhost URLs correctly, although I am fine with leaving that out of scope of this pull request. Isn't there any Node.js core API or perhaps an npm module that can solve this problem for us?

The built-in url API seems to work well

$ node
> require('url').format({protocol: 'http', hostname: '::1', port: 80, pathname: '/foo'})
'http://[::1]:80/foo'

The docs says this code is considered as legacy and should not be used in new code :(

Unfortunately the newer WhatWG API does not encode IPv6 local addressed as the old one, so I am not sure how much benefit we can get from using it.

See nodejs/help#1176 (comment)

const u = new (require('url').URL)('http://./');
u.protocol = 'https';
u.host = '[::1]';
u.port = 3000;
u.pathname = '/foo';
u.href 
// returns 
// 'https://[::1]:3000/foo'

-1 for Travis-specific hacks, see #1369 (comment)

We cannot ship workarounds for Travis CI in code that will be used by LB apps in production!

If IPv6 localhost addresses are causing problems in Travis, then let's fix our tests to explicitly listen on 127.0.0.1 instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@raymondfeng can we do the "robustification" after landing the basic functionality?

Copy link
Contributor

@raymondfeng raymondfeng Jun 7, 2018

Choose a reason for hiding this comment

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

If we force tests to only listen on 127.0.0.1, we then lose the coverage for host === undefined.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense. I am proposing to write two tests for host === undefined case:

  • One test to verify what is returned by host and url properties. I think we don't need to start the server for this and thus these tests can be at unit-test level.
  • One test to verify that server listening on all hosts accepts connections from 127.0.1. The trick is to build the request URL manually (http://127.0.0.1:${server.port}/) in the test, don't use server.url property.

}

/**
* State of the HTTP / HTTPS server
*/
public get started(): Boolean {
return this._started;
}

/**
* Address of the HTTP / HTTPS server
*/
public get address(): AddressInfo {
return this._address;
}
}
1 change: 1 addition & 0 deletions packages/http-server/src/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from './http-server';
125 changes: 125 additions & 0 deletions packages/http-server/test/integration/http-server.integration.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
// Copyright IBM Corp. 2018. All Rights Reserved.
// Node module: @loopback/http-server
// This file is licensed under the MIT License.
// License text available at https://opensource.org/licenses/MIT
import {HttpServer} from '../../';
import {supertest, expect} from '@loopback/testlab';
import * as makeRequest from 'request-promise-native';
import {ServerRequest, ServerResponse} from 'http';

describe('HttpServer (integration)', () => {
it('starts server', async () => {
const server = new HttpServer(dummyRequestHandler);
await server.start();
supertest(server.url)
.get('/')
.expect(200);
await server.stop();
});

it('stops server', async () => {
const server = new HttpServer(dummyRequestHandler);
await server.start();
await server.stop();
await expect(
makeRequest({
uri: server.url,
}),
).to.be.rejectedWith(/ECONNREFUSED/);
});

it('exports original port', async () => {
const server = new HttpServer(dummyRequestHandler, {port: 0});
expect(server)
.to.have.property('port')
.which.is.equal(0);
});

it('exports reported port', async () => {
const server = new HttpServer(dummyRequestHandler);
await server.start();
expect(server)
.to.have.property('port')
.which.is.a.Number()
.which.is.greaterThan(0);
await server.stop();
});

it('does not permanently bind to the initial port', async () => {
const server = new HttpServer(dummyRequestHandler);
await server.start();
const port = server.port;
await server.stop();
await server.start();
expect(server)
.to.have.property('port')
.which.is.a.Number()
.which.is.not.equal(port);
await server.stop();
});

it('exports original host', async () => {
const server = new HttpServer(dummyRequestHandler);
expect(server)
.to.have.property('host')
.which.is.equal(undefined);
});

it('exports reported host', async () => {
const server = new HttpServer(dummyRequestHandler);
await server.start();
expect(server)
.to.have.property('host')
.which.is.a.String();
await server.stop();
});

it('exports protocol', async () => {
const server = new HttpServer(dummyRequestHandler);
await server.start();
expect(server)
.to.have.property('protocol')
.which.is.a.String()
.match(/http|https/);
await server.stop();
});

it('exports url', async () => {
const server = new HttpServer(dummyRequestHandler);
await server.start();
expect(server)
.to.have.property('url')
.which.is.a.String()
.match(/http|https\:\/\//);
await server.stop();
});

it('exports address', async () => {
const server = new HttpServer(dummyRequestHandler);
await server.start();
expect(server)
.to.have.property('address')
.which.is.an.Object();
await server.stop();
});

it('exports started', async () => {
const server = new HttpServer(dummyRequestHandler);
await server.start();
expect(server.started).to.be.true();
await server.stop();
expect(server.started).to.be.false();
});

it('start() returns a rejected promise', async () => {
const serverA = new HttpServer(dummyRequestHandler);
await serverA.start();
const port = serverA.port;
const serverB = new HttpServer(dummyRequestHandler, {port: port});
expect(serverB.start()).to.be.rejectedWith(/EADDRINUSE/);
});

function dummyRequestHandler(req: ServerRequest, res: ServerResponse): void {
res.end();
}
});
8 changes: 8 additions & 0 deletions packages/http-server/tsconfig.build.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"$schema": "http://json.schemastore.org/tsconfig",
"extends": "../build/config/tsconfig.common.json",
"compilerOptions": {
"rootDir": "."
},
"include": ["index.ts", "src", "test"]
}
1 change: 1 addition & 0 deletions packages/rest/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
"@loopback/context": "^0.11.2",
"@loopback/core": "^0.8.4",
"@loopback/openapi-v3": "^0.10.5",
"@loopback/http-server": "^0.0.1",
"@loopback/openapi-v3-types": "^0.7.4",
"@types/cors": "^2.8.3",
"@types/express": "^4.11.1",
Expand Down
Loading