isomerpages / isomercms-backend

A static website builder and host for the Singapore Government
5 stars 1 forks source link

fix: only release the lock after the handler is done #1259

Closed timotheeg closed 6 months ago

timotheeg commented 6 months ago

Problem

For write routes, the site lock is released too early, which makes the lock useless

This is a sibling PR to https://github.com/isomerpages/isomercms-backend/pull/1256, which introduced the same fix for the rollback routes.

Solution

Only release the lock AFTER the action for the handler is completed.

Notes:

  1. For this to work (same a rollback routes), there is an assumption that ALL write route handlers are async and resolve when their job is done, but that is NOT enforced from typings. The handler is of type RequestHandler which is not specified to return a Promise, it returns void.
  2. The handling of the express flow still looks wrong and should be discussed. When the handler is successful, and the lock fails to release, does it really make sense to call next(err)? The query was handled successfully, and failure to release the lock could be retried or just logged (and tracked!)
timotheeg commented 6 months ago

I merged this first since it's a minimalistic fix that is obviously an improvement.

After release, we should both 1) monitor traces and, 2) inspect the code to verify that the handlers have valid async behaviour.

On 2) It's OK when some work is done fire-and-forget on purpose (e.g. git push), but we should still verify that all routes exhibit their intended async behaviour. There are not that many routes that use the wrappers, so hopefully doing static code analysis on them shouldn't take too long 🤞

git grep attachRollbackRouteHandlerWrapper | grep -v spec | wc -l
      54

git grep attachWriteRouteHandlerWrapper | grep -v spec | wc -l
      15