PYIC-7869: Spike - render service-unavailable page on 503 #1761
Conversation
The overload protection lib doesn't seem to be configurable to specify a template or html page to return when it kicks in. We can however set it to hand control back to the framework and handle a 503 ourselves. The downside of this is that it will require the app to do work when it's already overloaded.
| @@ -29,12 +29,16 @@ const serverErrorHandler: ErrorRequestHandler = (err, req, res, next) => { | |||
| return next(err); | |||
| } | |||
|
|
|||
| if (res.statusCode === 503) { | |||
| return res.render(getHtmlPath("errors", "service-unavailable-s3", ERROR_PAGES.SERVICE_UNAVAILABLE)) | |||
There was a problem hiding this comment.
Ah cool - I guess it's hard to tell whether routing through the error handlers will be substantially slower than the 'basic' response, but it feels like a worthwhile hit.
| @@ -29,12 +29,16 @@ const serverErrorHandler: ErrorRequestHandler = (err, req, res, next) => { | |||
| return next(err); | |||
| } | |||
|
|
|||
| if (res.statusCode === 503) { | |||
| return res.render(getHtmlPath("errors", "service-unavailable-s3", ERROR_PAGES.SERVICE_UNAVAILABLE)) | |||
There was a problem hiding this comment.
I didn't even realise we had this 503 page..! Perhaps once we get designs etc., we can build the page statically during the build process - something we'll worry about later.
| @@ -29,12 +29,16 @@ const serverErrorHandler: ErrorRequestHandler = (err, req, res, next) => { | |||
| return next(err); | |||
| } | |||
|
|
|||
| if (res.statusCode === 503) { | |||
| return res.render(getHtmlPath("errors", "service-unavailable-s3", ERROR_PAGES.SERVICE_UNAVAILABLE)) | |||
There was a problem hiding this comment.
Do you know if this will cache the file or load it from disk each time? That's something we probably want to watch out for when implementing for real
There was a problem hiding this comment.
By the look of it Express will cache views in production by default
There was a problem hiding this comment.
Which is interesting because it implies that lower envs won't cache the views. Which might not help the results of the perf tests.
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks - I suspect it's looking for NODE_ENV=production, which should always be the case in deployed environments. :)
I wonder if we should be using res.send or res.sendFile instead? I'm not totally sure what the view engine will do with raw HTML 🤔 - presumably it works though!
There was a problem hiding this comment.
Ah you're right - i just looked and it is production across the envs.
And yeah render works fine - but I guess there might be some perf gains for using send... I'll have a quick look
There was a problem hiding this comment.
So it looks like sendFile won't cache the file in memory - it'll read it from disk each time. So render might actually be a better choice, unless you want to introduce some other sort of caching.
Although I notice that we're globally setting Cache-control private no-store (among others) on all responses. Maybe if we allow caching, cloudfront will handle it for us?
There was a problem hiding this comment.
Ah - but we probably don't want CloudFront caching /page/ipv-success-page as a 503 error screen
If render 'just works' then that might be easiest. Otherwise we can load the file on startup and serve it from memory.
Proposed changes
What changed
Spike - render service-unavailable page on 503
Why did it change
The overload protection lib doesn't seem to be configurable to specify a template or html page to return when it kicks in. We can however set it to hand control back to the framework and handle a 503 ourselves.
The downside of this is that it will require the app to do work when it's already overloaded.
Issue tracking