Ensure All Newsletters page is async #26873
Conversation
…int to rendering app
| def renderNewsletters()(implicit | ||
| executionContext: ExecutionContext = this.executionContext, | ||
| ): Action[AnyContent] = | ||
| csrfAddToken { | ||
| Action { implicit request => | ||
| if (request.forceDCR || UseDcrNewslettersPage.isSwitchedOn) { | ||
| renderDCRNewslettersJson() | ||
| } else { | ||
| renderNewslettersJson() | ||
| Action.async { implicit request => | ||
| val useDCR = request.forceDCR || UseDcrNewslettersPage.isSwitchedOn | ||
|
|
||
| request.getRequestFormat match { | ||
| case HtmlFormat if useDCR => | ||
| remoteRenderNewslettersPage() | ||
| case HtmlFormat => | ||
| localRenderNewslettersPage() | ||
| case JsonFormat if useDCR => | ||
| renderDCRNewslettersJson() | ||
| case _ => notFoundPage() |
There was a problem hiding this comment.
This function combines request handling for both JSON and HTML requests, removing some duplicated code above and (I believe) making it a bit easier to read.
| } | ||
| case Left(e) => | ||
| log.error(s"API call to get newsletters failed: $e") | ||
| throw new RuntimeException() | ||
| } | ||
| } | ||
|
|
||
| private def renderNewslettersJson()(implicit | ||
| private def notFoundPage()(implicit |
There was a problem hiding this comment.
Named more appropriately, I think? Might have got the wrong end of the stick though...
| def renderNewslettersPage()(implicit | ||
| executionContext: ExecutionContext = this.executionContext, | ||
| ): Action[AnyContent] = | ||
| csrfAddToken { | ||
| Action { implicit request => | ||
| if (request.forceDCR || UseDcrNewslettersPage.isSwitchedOn) { | ||
| remoteRenderNewslettersPage() | ||
| } else { | ||
| localRenderNewslettersPage() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Moved into generic renderNewsletters function at bottom of page which handles both JSON and HTML and DCR / non DCR requests
| Await.result( | ||
| remoteRenderer.getEmailNewsletters( | ||
| ws = wsClient, | ||
| newsletters = newsletters, | ||
| page = StaticPages.dcrSimpleNewsletterPage(request.path), | ||
| ), | ||
| 3.seconds, |
There was a problem hiding this comment.
I think the presence of this extra Await could be the reason why the DCR circuit breaker keeps opening. The DotcomRenderingService has a timeout of 2 seconds so looks like this could be potentially blocking operations for one second after the DCR timeout and before the Await atMost duration of 3 seconds?
| } else { | ||
| renderNewslettersJson() | ||
| Action.async { implicit request => | ||
| val useDCR = request.forceDCR || UseDcrNewslettersPage.isSwitchedOn |
There was a problem hiding this comment.
Not a blocker as this is the current behaviour, but with this logic, it’s impossible to force DCR off when the UseDcrNewslettersPage switch is on.
There was a problem hiding this comment.
Ah, yep you're right. I actually wonder if that's intentional as the frontend newsletters page is VERY different to the DCR one 🤔
|
Seen on FRONTS-PROD (merged by @cemms1 12 minutes and 14 seconds ago)
|
What is the value of this and can you measure success?
We're getting lots of errors on the
faciaapp since directing the DCR requests for/EmailNewsletterstofacia-rendering, so this refactors the code to render the all newsletters page and (hopefully) fixes an issue with a blocking operation.What does this change?
SignupPageControllerto beAction.asyncrather than justActionand as such, swaps the return types to beFuture[Result]instead of justResult.Awaitfrom the DCR request code, along with the max duration of 3 seconds.In the docs for
Awaitit says: