Skip to content

Commit d3bcf6e

Browse files
ryanoleeryan lee
andauthored
feat: overhaul error handling
deal with most hanging request issues (#193) * Improve boom error handling to make sure requests don't hang * Overhaul error handling * Update package lock and run linting Co-authored-by: ryan lee <rol@bluetel.co.uk>
1 parent 023470f commit d3bcf6e

File tree

9 files changed

+34194
-15682
lines changed

9 files changed

+34194
-15682
lines changed

package-lock.json

Lines changed: 34079 additions & 15650 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

package.json

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -48,13 +48,12 @@
4848
],
4949
"dependencies": {
5050
"body-parser": "^1.19.0",
51-
"boom": "^7.3.0",
5251
"connect": "^3.7.0",
5352
"cookie-parser": "^1.4.5",
5453
"flat-cache": "^1.3.2",
5554
"fs-extra": "^9.0.0",
5655
"glob-to-regexp": "^0.4.1",
57-
"http-status-codes": "^1.4.0"
56+
"http-status-codes": "^2.1.4"
5857
},
5958
"devDependencies": {
6059
"@commitlint/cli": "^8.2.0",
@@ -66,7 +65,6 @@
6665
"@semantic-release/release-notes-generator": "^7.3.0",
6766
"@types/aws-lambda": "^8.10.51",
6867
"@types/body-parser": "^1.19.0",
69-
"@types/boom": "^7.2.1",
7068
"@types/connect": "^3.4.32",
7169
"@types/cookie-parser": "^1.4.2",
7270
"@types/fs-extra": "^9.0.1",

src/behavior-router.ts

Lines changed: 19 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,14 @@
11
import { Context } from 'aws-lambda';
22
import bodyParser from 'body-parser';
3-
import { isBoom } from 'boom';
4-
import * as Boom from 'boom';
53
import connect, { HandleFunction } from 'connect';
64
import cookieParser from 'cookie-parser';
75
import * as fs from 'fs-extra';
86
import { createServer, IncomingMessage, ServerResponse } from 'http';
9-
import { NOT_FOUND, OK } from 'http-status-codes';
7+
import { StatusCodes } from 'http-status-codes';
108
import * as os from 'os';
119
import * as path from 'path';
1210
import { URL } from 'url';
11+
import { HttpError, InternalServerError } from './errors/http';
1312

1413
import { FunctionSet } from './function-set';
1514
import { asyncMiddleware, cloudfrontPost } from './middlewares';
@@ -92,7 +91,7 @@ export class BehaviorRouter {
9291
if ((req.method || '').toUpperCase() === 'PURGE') {
9392
await this.purgeStorage();
9493

95-
res.statusCode = OK;
94+
res.statusCode = StatusCodes.OK;
9695
res.end();
9796
return;
9897
}
@@ -101,7 +100,7 @@ export class BehaviorRouter {
101100
const cfEvent = convertToCloudFrontEvent(req, this.builder('viewer-request'));
102101

103102
if (!handler) {
104-
res.statusCode = NOT_FOUND;
103+
res.statusCode = StatusCodes.NOT_FOUND;
105104
res.end();
106105
return;
107106
}
@@ -111,7 +110,7 @@ export class BehaviorRouter {
111110
const response = await lifecycle.run(req.url as string);
112111

113112
if (!response) {
114-
throw Boom.internal();
113+
throw new InternalServerError('No response set after full request lifecycle');
115114
}
116115

117116
res.statusCode = parseInt(response.status, 10);
@@ -127,10 +126,8 @@ export class BehaviorRouter {
127126

128127
res.end(response.body);
129128
} catch (err) {
130-
if (isBoom(err)) {
131-
this.handleError(err, res);
132-
return;
133-
}
129+
this.handleError(err, res);
130+
return;
134131
}
135132
}));
136133

@@ -147,11 +144,19 @@ export class BehaviorRouter {
147144
}
148145
}
149146

150-
public handleError(err: Boom<any>, res: ServerResponse) {
151-
res.statusCode = err.output.statusCode;
152-
res.statusMessage = err.output.payload.error;
147+
// Format errors
148+
public handleError(err: HttpError, res: ServerResponse) {
149+
res.statusCode = err.statusCode || StatusCodes.INTERNAL_SERVER_ERROR;
150+
151+
const payload = JSON.stringify(err.hasOwnProperty('getResponsePayload') ?
152+
err.getResponsePayload() :
153+
{
154+
code: StatusCodes.INTERNAL_SERVER_ERROR,
155+
message: err.stack || err.message
156+
}
157+
);
153158

154-
res.end(err.message);
159+
res.end(payload);
155160
}
156161

157162
public async purgeStorage() {

src/errors/http/http.error.ts

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
import { getReasonPhrase } from 'http-status-codes';
2+
3+
interface HttpErrorResponse {
4+
code: number;
5+
message: string;
6+
}
7+
8+
export class HttpError extends Error {
9+
public statusCode: number;
10+
public message: string;
11+
public reasonPhrase: string|number;
12+
public originalError: Error|null;
13+
14+
constructor(message: string, statusCode: number, originalError: Error|null = null) {
15+
super(message);
16+
this.message = message;
17+
this.statusCode = statusCode;
18+
19+
try {
20+
this.reasonPhrase = getReasonPhrase(statusCode);
21+
} catch {
22+
this.reasonPhrase = 'Unknown';
23+
}
24+
25+
this.originalError = originalError;
26+
}
27+
28+
public getResponsePayload(): HttpErrorResponse {
29+
return {
30+
code: this.statusCode,
31+
message: this.message
32+
};
33+
}
34+
}

src/errors/http/index.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
export * from './internal-server.error';
2+
export * from './not-found.error';
3+
export * from './http.error';
Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
import { StatusCodes } from 'http-status-codes';
2+
import { HttpError } from './http.error';
3+
4+
export class InternalServerError extends HttpError {
5+
constructor(message: string, lastError: Error|null = null) {
6+
super(message, StatusCodes.INTERNAL_SERVER_ERROR, lastError);
7+
}
8+
}

src/errors/http/not-found.error.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
import { StatusCodes } from 'http-status-codes';
2+
import { HttpError } from './http.error';
3+
4+
export class NotFoundError extends HttpError {
5+
constructor(message: string, lastError: Error|null = null) {
6+
super(message, StatusCodes.NOT_FOUND, lastError);
7+
}
8+
}

src/function-set.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,11 @@ export class FunctionSet {
5656

5757
const handler = async (event: CloudFrontRequestEvent, context: Context) => {
5858
const promise = new CallbackPromise();
59+
60+
if (typeof fn !== 'function') {
61+
throw new Error(`Unable to find request handler under path: ${fn}. Please recheck your serverless.yml / exported handlers!`);
62+
}
63+
5964
const result = fn(event, context, promise.callback) as CloudFrontRequestResult;
6065

6166
if (result instanceof Promise) {

src/services/origin.service.ts

Lines changed: 37 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
import { CloudFrontRequest, CloudFrontRequestEvent, CloudFrontResultResponse } from 'aws-lambda';
2-
import * as Boom from 'boom';
32
import * as fs from 'fs-extra';
43
import * as http from 'http';
54
import * as https from 'https';
@@ -8,6 +7,8 @@ import * as path from 'path';
87
import { parse } from 'url';
98
import { toHttpHeaders } from '../utils';
109
import { OutgoingHttpHeaders } from 'http';
10+
import { InternalServerError, NotFoundError } from '../errors/http';
11+
import { StatusCodes } from 'http-status-codes';
1112

1213

1314
export class Origin {
@@ -36,7 +37,7 @@ export class Origin {
3637

3738
return {
3839
status: '200',
39-
statusDescription: '',
40+
statusDescription: 'OK',
4041
headers: {
4142
'content-type': [
4243
{ key: 'content-type', value: 'application/json' }
@@ -46,16 +47,23 @@ export class Origin {
4647
body: contents
4748
};
4849
} catch (err) {
49-
if (err instanceof Boom.notFound) {
50-
return {
51-
status: '404'
52-
};
53-
} else {
54-
return {
55-
status: '500',
56-
statusDescription: err.message
57-
};
58-
}
50+
// Make sure error gets back to user
51+
const status = err.statusCode || StatusCodes.INTERNAL_SERVER_ERROR;
52+
const reasonPhrase = err.reasonPhrase || 'Internal Server Error';
53+
return {
54+
status: status,
55+
statusDescription: reasonPhrase,
56+
headers: {
57+
'content-type': [
58+
{ key: 'content-type', value: 'application/json' }
59+
]
60+
},
61+
bodyEncoding: 'text',
62+
body: JSON.stringify({
63+
'code': status,
64+
'message': err.message
65+
})
66+
};
5967
}
6068
}
6169

@@ -68,13 +76,13 @@ export class Origin {
6876
}
6977
case 'http':
7078
case 'https': {
71-
return this.getHttpResource(request);
79+
return await this.getHttpResource(request);
7280
}
7381
case 'noop': {
74-
throw Boom.notFound();
82+
throw new NotFoundError('Operation given as \'noop\'');
7583
}
7684
default: {
77-
throw Boom.internal('Invalid origin type');
85+
throw new InternalServerError('Invalid request type (needs to be \'http\', \'https\' or \'file\')');
7886
}
7987
}
8088
}
@@ -83,6 +91,20 @@ export class Origin {
8391
const uri = parse(key);
8492
const fileName = uri.pathname;
8593

94+
const fileTarget = `${this.baseUrl}/${fileName}`;
95+
96+
// Check for if path given is accessible and is a file before fetching it
97+
try {
98+
await fs.access(fileTarget);
99+
} catch {
100+
throw new NotFoundError(`File ${fileTarget} does not exist`);
101+
}
102+
103+
const fileState = await fs.lstat(fileTarget);
104+
if (!fileState.isFile()) {
105+
throw new NotFoundError(`${fileTarget} is not a file.`);
106+
}
107+
86108
return await fs.readFile(`${this.baseUrl}/${fileName}`, 'utf-8');
87109
}
88110

0 commit comments

Comments
 (0)