context: AppIfConfigured returns error; consider not-yet-provisioned modules #6292
Conversation
|
We talked about that change though. Instantiating an app when it wasn't loaded was problematic. |
|
Ok, discussed on Slack, and quoting your summary here:
|
context.go
Outdated
| // Since this method does not return the app if it has not been | ||
| // provisioned, this could be misleading if the provisioning order | ||
| // caused the app to not be provisioned yet. This could return | ||
| // nil even if the app is configured, but not yet provisioned. | ||
| // In that case, the caller should probably use AppStrict() instead | ||
| // to ensure the app is provisioned, and no app will be instantiated | ||
| // if it is not configured. |
There was a problem hiding this comment.
So the more I read this, the more I think we need to just fix this: if the app has been configured, load and provision it if needed. Otherwise, don't. This method is called AppIfConfigured not AppIfProvisionedAlready...
What do you think if we try to do that?
There was a problem hiding this comment.
Assuming you had a particular need when you changed it in 0e2c7e1, are those semantics not needed anymore?
There was a problem hiding this comment.
I don't think either change is quite right though...
The earlier version would provision an app that maybe shouldn't be provisioned (I forgot about the nondeterminstic order). The change makes it never provision the app, but also disregards the loading order.
AppIfConfigured should provision the app if, but ONLY if, it will be provisioned.
There was a problem hiding this comment.
I don't think I follow. So what do you think should be done? Just rename AppStrict to AppIfConfigured (and drop the current one & functionality)?
Why did the difference matter for the cert cache commit again? Can you give an example of when it would have been wrong?
There was a problem hiding this comment.
I'm a dummy, AppStrict() already does exactly what I'm describing, lol.
So yeah, let's replace AppIfConfigured with AppStrict. This does add an error return to the signature but we had it originally and I don't think there's anyone using this yet? (maybe 1 or 2? I apologize if that breaks anyone, but it's a very very easy patch, and I think this is an important fix).
This summary is pretty neat. It'd be great if we can have it documented this way somewhere. |
mholt
changed the title
AppStrict() method to avoid instantiating empty apps
Context https://caddy.community/t/app-not-available-during-provisioning/23678
The problem is that our two existing methods
ctx.App(name)andctx.AppIfConfigured(name)don't properly cover the case where a module might want to get a configured module instance, becauseAppIfConfigured()could be called before the configured app is actually provisioned, andApp()could provision an empty app even though it wasn't configured.So to plug that gap, I propose we add an
ctx.AppStrict(name)method which is the same asctx.App(name)except that it throws an error early if the app wasn't configured. If it was already loaded then it just returns that, if configured it just loads it and returns it. The method's name is open for discussion if anyone can think of something better.I also realized that a couple of our usages of
AppIfConfigured()were error prone due to provisioning order not being guaranteed, so I switched them to use this new method.There's still one remaining case where we use
AppIfConfigured()which is in the TLS app's cleanup, we check if the next config (which must have been fully started/provisioned before cleanup of the first config happens) also has a TLS app, and if so pass stuff along. That's to sayAppIfConfigured()still has a purpose for those kinds of handoff scenarios.