webapi: Abort requests if web cache not ready. #440
Conversation
A new middleware aborts any requests which require the data cache if the cache is not yet initialized. An explicit error is returned so the admin will be immediately aware what has gone wrong. Previously, webpages were rendered and JSON responses were sent with zero values, and with no indication of anything being wrong. This was sometimes difficult to notice, and even when noticed the cause was not immediately apparent.
There was a problem hiding this comment.
This is a great improvement. It's much nicer to get explicit errors.
That said, and it could just be because I'm not super familiar with this code base yet, but why not just store the cache in the WebAPI type and require the cache to be initialized its constructor (returning an error if it fails to initialize there) as opposed to stuffing it into the gin context where it has to constantly be checked, needs an additional middleware, etc?
|
The cache failing to initialize shouldn't be a total showstopper - the vspd process should continue startup as normal so the rest of its functions can occur (tickets are adding to voting wallets etc). It's a recoverable error - chances are the cache will update sooner or later - e.g. once the RPC connection to dcrd is re-established. As for why to use a middleware:
|
Rather than immediately returning an error if the cache is not initialized, attempt to initialize it first. Without this change there is only an attempt to initialize the cache once per minute.
|
Pushed one more commit which attempts to initialize the cache before returning an error. |
Alright, got it. Thanks for the explanation. I understand and agree with the use of a middleware to require it with each request. Ergo, instead of It's probably not a big deal, but I really don't like stuffing things into contexts in general when it can be avoided. It loses type safety and tends to hide dependencies. EDIT: I'm fine with approving it as is if you disagree, btw. I don't see any issue that would prevent it from working properly. It's just more about trying to guard against potential future issues in refactors. |
|
Your suggestion is correct, that would work and be safe. And while I do agree that the use of Context has significant drawbacks, in this particular case it actually does offer a small benefit in return. Retrieving from context implies that it must have been added to context - ie. it ensures that the middleware has been run and initialization has been checked. Accessing it directly offers no such guarantee, there would be no hard check that middleware has definitely been executed. When context is used, middleware misconfiguration results in a development-time panic. When it isn't, the same misconfiguration results in a "silent failure" which can only be noticed by manual code review. |
|
The panic was actually precisely my concern (hence the comment about loss of type safety)! The cache isn't mandatory, but a slight configuration mistake will lead to the entire VSP going down at run time. However, I guess it wouldn't be too bad since the voting wallets are separate and thus it would only be the front end. |
A new middleware aborts any requests which require the data cache if the cache is not yet initialized. An explicit error is returned so the admin will be immediately aware what has gone wrong.
Previously, webpages were rendered and JSON responses were sent with zero values, and with no indication of anything being wrong. This was sometimes difficult to notice, and even when noticed the cause was not immediately apparent.