fix: optimize serving static files #1848

hacksparrow · 2018-10-15T04:36:51Z

Optimize serving static files

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-10-15T08:34:25Z

packages/rest/src/router/routing-table.ts

@@ -290,7 +290,7 @@ export class Route extends BaseRoute {
    verb: string,
    path: string,
    public readonly spec: OperationObject,
-    protected readonly _handler: Function,
+    protected readonly _handler: Function, // <-- doesn't this Function have a signature?


The handler function accepts a list of parameters as described in its OpenAPI specification.

See invokeHandler below and our docs: https://loopback.io/doc/en/lb4/Routes.html#using-partial-openapi-spec-fragments

bajtos · 2018-10-15T08:39:06Z

packages/rest/src/rest.server.ts

+    // LB4's static assets serving router
+    const staticAssetsRouter = new Route('get', '*', {responses: {}}, () => {
+      console.log('>> staticAssetsRouter HANDLER');
+      // @bajtos How do we access req and res here?


You cannot access the request/response from a handler route (a Route instance). In my comment #1784 (comment), I was showing how to create a new kind of RouteEntry (a new class implementing RouteEntry interface). RouteEntry's method invokeHandler does have access to the request/response objects.

// somewhere in the code setting up RoutingTable // the anonymous RouteEntry object should be refactored into a class this.routingTable.catchAllRoute = <RouteEntry>{ verb: 'get', path: '*', spec: { /* TODO: this route should be excluded from the spec */ }, updateBindings(requestContext: Context) { // no-op }, invokeHandler( {request, response}: Context, args: OperationArgs, ): Promise<OperationRetval> { return this._handleStaticAssetRequest(request, response); } describe(): string { return "final route to handle static assets"; } };

bajtos

I don't like the fact that in the current design, the implementation of StaticAsset routing is scattered across multiple files and layers of abstraction (server, router, routing table, StaticRoute). Let's try to find a way how to keep most of the implementation in one or two places please - see my comments below.

bajtos · 2018-10-16T10:02:54Z

packages/rest/src/router/routing-table.ts

+    args: OperationArgs,
+  ): Promise<OperationRetval> {
+    const req = await requestContext.get(RestBindings.Http.REQUEST);
+    const res = await requestContext.get(RestBindings.Http.RESPONSE);


Let's use context.request and context.response, it should be faster than obtaining the values via Context bindings.

bajtos · 2018-10-16T10:05:51Z

packages/rest/src/router/routing-table.ts

+  ): Promise<OperationRetval> {
+    const req = await requestContext.get(RestBindings.Http.REQUEST);
+    const res = await requestContext.get(RestBindings.Http.RESPONSE);
+    return this._handler(req, res);


I am proposing to simplify the design here. Since there will be always at most one StaticRoute instance registered with a server, there is no need to make the handler configurable. Let's move the code dealing with _routerForStaticAssets here into invokeHandler.

Ideally, I'd like to see _routerForStaticAssets to be a member property of StaticRoute.

bajtos · 2018-10-16T10:07:13Z

packages/rest/src/rest.server.ts

+              return reject(err);
+            }
+            // Express router called next, which means no route was matched
+            return reject(new HttpErrors.NotFound());


Let's be more verbose here. See how we deal with 404 errors now:

https://github.com/strongloop/loopback-next/blob/a2ab86d3a3c721f28159585ca4e87bd8504476c4/packages/rest/src/router/routing-table.ts#L183-L186

bajtos · 2018-10-16T10:11:54Z

packages/rest/src/rest.server.ts

+      },
+    );
+
+    this.httpHandler.registerRoute(staticAssetsRouter);


IIUC, this is relying on the fact that the code paths invoked here is bypassing path validations and that our routing table will pass the path * to Express router without modifications.

I find that rather brittle, let's find a different solution please.

I am thinking about the following implementation of RoutingTable#find:

class RoutingTable { // ... find(request: Request): ResolvedRoute { debug('Finding route %s for %s %s', request.method, request.path); const found = this._router.find(request); if (found) { debug('Route matched: %j', found); return found; } debug('No API route found for %s %s, trying to find a static assets', request.method, request.path); return this._staticAssetsRoute; } }

bajtos · 2018-10-16T10:15:05Z

packages/rest/src/router/routing-table.ts

@@ -238,6 +240,31 @@ export interface ResolvedRoute extends RouteEntry {
  readonly schemas: SchemasObject;
 }

+export class StaticRoute implements RouteEntry {


Let's call this StaticAssetsRoute. The name "static route" can be also interpreted as a route that's static (e.g. does not accept any path parameters).

hacksparrow · 2018-10-16T12:25:30Z

Thanks @bajtos, I will be updating the implementation based on your feedback.

raymondfeng · 2018-10-17T15:36:07Z

packages/rest/src/router/routing-table.ts

@@ -150,6 +157,9 @@ export class RoutingTable {

    validateApiPath(route.path);
    this._router.add(route);
+    if (route.path === '*') {


This is a bit hacky to test path === '*'. The special route is also added to the underlying router, which might cause unexpected results.

bajtos · 2018-10-18T13:20:59Z

@hacksparrow please let me know when this pull request is ready for another review. Also the title says [WIP], is it still true?

hacksparrow · 2018-10-18T13:29:34Z

@bajtos I will let you now. There is API spec generation side-effect, please consider it WIP till I remove it.

bajtos · 2018-10-18T14:08:02Z

@hacksparrow are you referring to the following part of #1784 (comment)? It would be best to open a new pull request for that, to make the review easier and get the new flag lander faster.

The last missing piece I see is how to exclude a route from the generated API spec. Personally, I'd introduce an extension property allowing developers to mark any route as not documented. For example:

const spec: OperationObject = {
  'x-internal': true,

  description: 'return all customers',
  parameters: [
    // ...
  ],
  responses: {
    // ...
  },
};

Few alternatives to x-internal: true

x-documented: false (in LB 3.x, remoting metadata has documented: false)

x-private: true

hacksparrow · 2018-10-18T14:13:27Z

@bajtos yes that's what I am referring too. Let me add that feature first then. Breakpoints jumping locations and disappearing is making debugging really hard.

Optimize serving static files

review 1

hacksparrow · 2018-10-24T17:55:42Z

packages/rest/src/router/routing-table.ts

@@ -150,6 +160,9 @@ export class RoutingTable {

    validateApiPath(route.path);
    this._router.add(route);
+    if (route.path === '/*') {


@raymondfeng you had some reservations about this.

This is a bit hacky to test path === '*'. The special route is also added to the underlying router, which might cause unexpected results.

Can you elaborate the concerns, so that we can start a discussion?

On line 162, the special route is added to the router implementation, which probably doesn't know /*.

I am still not able to see what the potential problem is. We run validateApiPath(route.path) and /* is a valid key to the static assets router. What am I missing?

It's not helpful to have /* in the router which won't match any requests.

In your implementation, the static asset route is a special one outside of the Router responsibility. It serves as the last matching candidate.

So do we use a non-path key, like path = 'catch-all-router'? We'll have to skip path validation in that case. What are your suggestions?

@bajtos what are your thoughts on this?

I'm fine to have /* as the path value, but don't call this._router.add(route);.

Got it now.

hacksparrow · 2018-10-24T18:07:16Z

packages/rest/src/rest.server.ts

@@ -58,6 +58,10 @@ export interface HttpServerLike {
  requestHandler: HttpRequestListener;
 }

+interface ExpressRouter extends express.Router {


ExpressRouter is used in rest.server.ts and routing-table.ts, and defined in both the files. What is a good location to export it from to avoid duplication?

Having to extend express.Router because its definition file does not include the handle method for it. I wonder why.

Please note that express.Router itself is the handle function.

src/types.ts is a good place to define common interfaces

As @raymondfeng said, express.Router is a callable function. It extends IRouter which extends RequestHandler which is defined as follows:

export interface RequestHandler { // tslint:disable-next-line callable-types (This is extended from and can't extend from a type alias in ts<2.2 (req: Request, res: Response, next: NextFunction): any; }

raymondfeng · 2018-10-25T05:19:58Z

packages/rest/src/rest.server.ts

@@ -242,8 +246,13 @@ export class RestServer extends Context implements Server, HttpServerLike {
   */
  protected _setupRouterForStaticAssets() {
    if (!this._routerForStaticAssets) {
-      this._routerForStaticAssets = express.Router();
-      this._expressApp.use(this._routerForStaticAssets);
+      this._routerForStaticAssets = express.Router() as ExpressRouter;


I don't think this is a valid cast as express.Router() returns a function which is a handler.

bajtos

The implementation looks mostly reasonable, let's improve the way how StaticAssetsRoute is integrated with server/handler/router parts.

bajtos · 2018-10-25T07:01:21Z

packages/rest/src/rest.server.ts

@@ -58,6 +58,10 @@ export interface HttpServerLike {
  requestHandler: HttpRequestListener;
 }

+interface ExpressRouter extends express.Router {


As @raymondfeng said, express.Router is a callable function. It extends IRouter which extends RequestHandler which is defined as follows:

export interface RequestHandler { // tslint:disable-next-line callable-types (This is extended from and can't extend from a type alias in ts<2.2 (req: Request, res: Response, next: NextFunction): any; }

bajtos · 2018-10-25T07:04:00Z

packages/rest/src/rest.server.ts

@@ -127,7 +131,7 @@ export class RestServer extends Context implements Server, HttpServerLike {
  protected _httpServer: HttpServer | undefined;

  protected _expressApp: express.Application;
-  protected _routerForStaticAssets: express.Router;
+  protected _routerForStaticAssets: ExpressRouter;


Let's move the implementation details of dealing with static assets down to StaticAssetsRoute please.

Ideally, I'd like to see RestServer#static implemented as a thin wrapper delegating the registration of static-asset middleware to StaticAssetsRoute and/or our HttpHandler class.

See also my earlier #1848 (comment)

@bajtos Calling app.static after the server has started should still set up the static assets route successfully. I tried to understand what you are suggesting and moved some pieces around, but resulted in app.static losing functionality after the app started.

Can you elaborate more on your suggestion, please? What does the outline of RestServer#static look like?

Let's wait until you have addressed the other comments and then I can try to shuffle the code around myself, I think it will be the fastest option.

I had addressed all others, except this one. Please refresh to see the changes.

bajtos · 2018-10-25T07:12:04Z

packages/rest/src/router/routing-table.ts

+        const onFinished = () => resolve();
+        response.once('finish', onFinished);
+        this._routerForStaticAssets.handle(request, response, (err: Error) => {
+          if (err) {


When the router called us back, then please remove onFinished listener to avoid resolving/rejecting the promise twice and also to avoid a potential (memory) leak.

It's a once, do we still need to remove it?

Yes, because when the static asset cannot be found or a ReadStream cannot be created, then the finish event is not fired within the timeframe of this async method.

I guess one could argue that the finish method will be eventually fired because the HTTP response will be finished by other parts of the framework eventually, but I find it very brittle and hacky to rely on some other part of our codebase to trigger "finish". See also Action at a distance anti-pattern.

bajtos · 2018-10-25T07:14:21Z

packages/rest/src/router/routing-table.ts

+  updateBindings(requestContext: Context): void {
+    // no-op
+  }
+  async invokeHandler(


No need to use async if you are not calling await.

bajtos · 2018-10-25T07:15:10Z

packages/rest/src/router/routing-table.ts

+    args: OperationArgs,
+  ): Promise<OperationRetval> {
+    const {request, response} = requestContext;
+    return this._handler(request, response);


Have you considered inlining this._handler inside invokeHandler?

class StaticAssetsRoute /*...*/ { //... invokeHandler( {request, response}: RequestContext, args: OperationArgs, ): Promise<OperationRetval> { return new Promise((resolve, reject) => { const onFinished = () => resolve(); response.once('finish', onFinished); // ... }); } //... }

bajtos · 2018-10-25T07:16:59Z

packages/rest/test/integration/rest.server.integration.ts

@@ -164,7 +147,7 @@ describe('RestServer (integration)', () => {
      .expect(200, 'Hello');
  });

-  it('serve static assets if matches before other routes', async () => {
+  it('does not serve static assets even if matches before other routes', async () => {


I am proposing to rephrase this test name as follows:

it('gives precedence to API routes over static assets')

review 2

bajtos · 2018-10-26T08:18:55Z

@hacksparrow I pushed a new commit f516b31 to further clean up the implementation, PTAL.

bajtos · 2018-10-26T08:22:27Z

One more improvement: 79b268c

bajtos

LGTM. Please make sure to clean up the git history before landing.

It would be great if you could get @raymondfeng's approval too.

bajtos · 2018-10-26T08:32:26Z

@hacksparrow hmm, the code coverage data suggests that we are missing the following test:

Create an app with static assets configured.
Request an URL that does not exist.
Verify that 404 was returned.

(This triggers a different code path than for a pure API server with no static assets.)

Could you please add it as part of this pull request?

hacksparrow · 2018-10-26T12:34:37Z

Will do.

Tests for RestApplication

raymondfeng · 2018-10-26T18:59:17Z

@hacksparrow I merged the PR so that we can do a release soon.

hacksparrow self-assigned this Oct 15, 2018

hacksparrow requested review from bajtos and raymondfeng as code owners October 15, 2018 04:36

hacksparrow force-pushed the option1 branch 2 times, most recently from 6feb7d9 to 99d95f6 Compare October 15, 2018 07:10

bajtos reviewed Oct 15, 2018

View reviewed changes

hacksparrow changed the title ~~[DO NOT MERGE] fix: optimize serving static files~~ [WIP] fix: optimize serving static files Oct 15, 2018

hacksparrow force-pushed the option1 branch 2 times, most recently from 2c9ec52 to acb0089 Compare October 16, 2018 03:47

bajtos suggested changes Oct 16, 2018

View reviewed changes

raymondfeng reviewed Oct 17, 2018

View reviewed changes

hacksparrow force-pushed the option1 branch from feef4e3 to 0ac2928 Compare October 18, 2018 05:06

hacksparrow mentioned this pull request Oct 18, 2018

Add new extension property to hide controllers and routes from openapi.json #1874

Closed

bajtos mentioned this pull request Oct 23, 2018

feat: add 'x-visibility' extension property to OpenAPI spec #1896

Merged

7 tasks

Hage Yaapa added 2 commits October 24, 2018 11:20

fix: optimize serving static files

89e8361

Optimize serving static files

fix: review 1

97bd294

review 1

hacksparrow force-pushed the option1 branch from 0ac2928 to 97bd294 Compare October 24, 2018 17:53

hacksparrow commented Oct 24, 2018

View reviewed changes

hacksparrow changed the title ~~[WIP] fix: optimize serving static files~~ fix: optimize serving static files Oct 24, 2018

hacksparrow commented Oct 24, 2018

View reviewed changes

raymondfeng reviewed Oct 25, 2018

View reviewed changes

bajtos suggested changes Oct 25, 2018

View reviewed changes

Hage Yaapa and others added 2 commits October 25, 2018 16:28

fix: review 2

a13be07

review 2

fixup! cleanup

f516b31

fixup! improve debug logs

79b268c

bajtos approved these changes Oct 26, 2018

View reviewed changes

hacksparrow force-pushed the option1 branch from a693732 to 94692d5 Compare October 26, 2018 14:31

raymondfeng approved these changes Oct 26, 2018

View reviewed changes

hacksparrow force-pushed the option1 branch from 94692d5 to 12e91d8 Compare October 26, 2018 14:52

fix: tests for RestApplication

17deb27

Tests for RestApplication

hacksparrow force-pushed the option1 branch from 12e91d8 to 17deb27 Compare October 26, 2018 17:24

raymondfeng merged commit 9c86e14 into master Oct 26, 2018

raymondfeng deleted the option1 branch October 26, 2018 18:59

This was referenced Oct 29, 2018

Error handling for non-API routes #1784

Closed

Invoke serve-static after API router #1785

Closed

bajtos mentioned this pull request Oct 30, 2018

Using Angular , React , VueJS #1818

Closed

6 tasks

fix: optimize serving static files #1848

fix: optimize serving static files #1848

Conversation

hacksparrow commented Oct 15, 2018 • edited by bajtos Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos left a 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

hacksparrow commented Oct 16, 2018

Choose a reason for hiding this comment

bajtos commented Oct 18, 2018

hacksparrow commented Oct 18, 2018

bajtos commented Oct 18, 2018

hacksparrow commented Oct 18, 2018

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 left a 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 Oct 26, 2018

bajtos commented Oct 26, 2018

bajtos left a comment

Choose a reason for hiding this comment

bajtos commented Oct 26, 2018

hacksparrow commented Oct 26, 2018

raymondfeng commented Oct 26, 2018

hacksparrow commented Oct 15, 2018 •

edited by bajtos

Loading