Add option to detect and error on dependency lifetime mismatch #349

fnimick · 2023-12-28T22:09:58Z

Adds new container option errorOnShorterLivedDependencies which enables detection and errors on cases where a longer-lived module resolves a shorter-lived dependency, which is then preserved inside the longer-lived module even after its own lifetime and cached resolution expires.

Fixes #348

NOTE: this changes the interface of ResolutionStack which was technically exported but should never be needed by client code. Does this constitute a breaking change for versioning purposes?

Adds new container option `errorOnShorterLivedDependencies` which enables detection and errors on cases where a longer-lived module resolves a shorter-lived dependency, which is then preserved inside the longer-lived module even after its own lifetime and cached resolution expires. Fixes #348

Confirms `errorOnShorterLivedDependencies` compatibility with per-module local injections.

fnimick · 2023-12-28T22:45:28Z

@jeffijoe I just realized this option causes a new problem. By default all values registered with asValue are transient, but the lifetime is basically meaningless since asValue closes over the provided value - every call to the value resolver will return the same instance. This means that modules can safely depend on 'transient' values, as long as those values are registered in the same or higher scope compared to the parent.

I propose making values have scoped lifetime by default, and allowing asValue to take resolver options as a second argument so a singleton lifetime can be explicitly specified if necessary. Having a singleton service depend on a value registered in a scope is still likely a logic error since the singleton will retain the reference to it after the scope is disposed.

jeffijoe

Fantastic work @fnimick !

Left a few comments but I definitely want to merge this feature. I have thought about this a few times but I decided against it mainly because Microsoft's container in C# behaves the same way, but having an option to warn against this rather than either enforcing or allowing it is even better.

I do think we want to make this a major release though since defining your own resolvers is not uncommon, and unless they specify a lifetime they will likely trigger this error.

Speaking of, perhaps we should have an example of triggering this error in the README, including the error message, to help people who may encounter this error with searchability.

src/lifetime.ts

src/resolvers.ts

src/container.ts

src/__tests__/lifetime.test.ts

coveralls · 2023-12-30T11:25:38Z

coverage: 100.0%. remained the same
when pulling 6d1832b on fnimick:fnimick/parent-lifetime-check
into 65b2de7 on jeffijoe:master.

fnimick · 2023-12-30T13:16:19Z

@jeffijoe re: "I do think we want to make this a major release though since defining your own resolvers is not uncommon, and unless they specify a lifetime they will likely trigger this error."

That's why I left this option as defaulting to "false", so that any current code that would trigger this error will continue to run as normal until the option is manually specified. That was my thought to avoid a breaking change necessitating a version bump.

Since it's very likely to be an error if this behavior is occurring, should we default this option to true, then do a major version bump instead?

jeffijoe · 2023-12-30T13:32:56Z

Good point; the resolution stack is an internal implementation detail so changing its' structure should be ok. I'm ok doing this as a minor version.

fnimick · 2023-12-30T13:45:25Z

@jeffijoe it's an approach question, not a "correctness" question per se. Do we want to have this be a default behavior such that people won't have to dig into the docs to realize they need to turn this option on for increased safety?

(at the very least, with the next breaking change, it should be made the default, I think)

jeffijoe · 2023-12-30T13:57:41Z

Yup I would agree with that. We'll make it opt-in for now in a minor version, then potentially make it opt-out in the next major, or maybe even eliminate the option depending on feedback (if any, lol).

README.md

fnimick · 2023-12-30T17:30:37Z

@jeffijoe developer UX question for you. There's a potentially unergonomic case that I discovered when modifying examples to use the new option - if you register a singleton service on the root container, and also a value, it errors since the value is registered as scoped. You need to manually register the value as a singleton in this case.

My suggestion is to treat values as singletons by default if and only if they are registered directly with the root container. I'm not entirely sure how to accomplish this yet - but would you support this? That way values registered on the root container don't need to be explicitly marked as singletons in order to be depended upon by singleton services.

There's a few separate questions that I have regarding how to approach the mixing of singleton and other lifetimes:

what should happen if you register a singleton both in the root and in a scope? the current behavior is that the singleton instance resulting from the first resolved container (either root or scope) is used for all future resolutions from either container.
what should happen if you register two singletons with the same name in two different scopes? the current behavior is that the resolution of one will leak to the other without warning.

Should we make it explicit that singletons should only ever be registered one the root container, and never on scopes?

EDIT: @jeffijoe if we added an optional restriction for singletons registered only on the root, would you support renaming this option from errorOnShorterLivedDependencies to strict and adding this restriction? That way we can have a strict mode to enforce multiple safety checks.

Also update all examples to use it (and fix examples where necessary)

jeffijoe · 2023-12-30T23:16:14Z

These are all good questions. strict definitely makes sense as the option name, it's not uncommon.

We could have an option on the resolver

{
  isLeakSafe?: boolean // name pending, suggestions?
}

And then any resolver that marks itself as true would be exempt from the check.

I wonder if things are becoming too complicated. Maybe this is why Microsoft's DI container doesn't do this. 😅

Thoughts?

fnimick · 2023-12-31T22:27:48Z

I'd say we should wait for someone to come with a compelling use case for a resolver to deliberately want to capture and leak information with a shorter lifetime before we allow the ability to disable it on a resolver-by-resolver basis. If you intend for a value to last as long as a singleton, just... register it as one.

I'd still like to align on answers to the other two questions - should it be illegal to register two singletons with the same name on different scopes from the same root container? (or in the root container in a scope?) to implement this I would have to add a check at register for the lifetime and to hoist all singletons to the root scope and register there, so I can detect the collision at registration time rather than at runtime where the source registration for the resolved cached module is no longer available for comparison (to detect if we are getting a singleton from a different registration than the one locally available)

jeffijoe · 2023-12-31T22:35:32Z

I would say it should be illegal to register a singleton on a non-root container.

Regarding the resolver option, my idea was that asValue could utilize it so we don’t need to special case it.

fnimick · 2024-01-01T19:30:25Z

Regarding the resolver option, my idea was that asValue could utilize it so we don’t need to special case it.

Ah, it wasn't clear (and still isn't, from the option name and the docs) whether this option would mean that: say A singleton depends on B transient. Would the isLeakSafe option be on A or B to disable the error in this case?

I assume you mean B, but it's not clear from the naming that that would be the case, and I think removing the need to have this be managed at all by the user would be an advantage.

Update `asValue()` to automatically set lifetime based upon registration container (root vs scope) Add checks to throw an error when a singleton is registered on a scope in strict mode

fnimick · 2024-01-01T20:29:39Z

@jeffijoe added strict mode + docs: enables checks for lifetime mismatch, and disallows registration of singletons on scope containers.

I also added intelligent value handling, so developers won't have to specify values as singletons when registering them on the root scope in order to have them be available to singleton modules. It detects whether the target container is a root container or a scope and sets the appropriate lifetime accordingly.

Note: I added "9.1" as the introductory version for strict mode in the README, assuming that's the version number we would use for this release.

README.md

jeffijoe · 2024-01-02T12:46:16Z

src/container.ts

+
+      // If this is a value resolver, set the lifetime to `Lifetime.SCOPED` if this is a scoped
+      // container, or `Lifetime.SINGLETON` if this is the root container.
+      if (resolver.isValue) {


This is what I meant when I was saying I would like to not special case asValue. Also, the aliasTo isn't covered, right? I should be able to register aliasTo() without specifying a lifetime (default being transient) since it'll essentially just "redirect" to the given registration.

My idea with the isLeakSafe (again, can workshop the name) was that we wouldn't need to tamper with the lifetime, but rather bypass the check that enforces the case where A depends on B, but A is singleton and B is scoped or transient (B would be leaking, since it lives longer than it was supposed to). However, if B is just an aliasTo, then it is safe to leak since when resolving the alias, the underlying registration will itself be subjected to the same checks. Or if it's asValue, then it's safe to leak because the value itself is already long-lived.

Thoughts?

That's an extremely good point. I was worried about aliasTo because really it needs to inherit the lifetime value of the registration it resolves to - but this is okay because we carry forward the check in the resolution stack. I was worried that there would be a mismatch between the lifetime defined at resolver creation time and the effective resolution which is undefined because aliasTo can resolve to a registration defined in a lower scope, and that therefore I couldn't give aliasTo a meaningful lifetime - but as you point out, we don't actually need to, since it's always a transient that resolves to a registration that has its lifetime checked.

However, I still think there is a problem where A is a singleton and B is a value defined at a lower scope. In this case, we want to detect this lifetime error. The analogous case where A is a singleton and B is a value defined at the same root scope is not an error. That's why values need special behavior that takes into account the container level they are registered at - it's not sufficient to always exclude values from leak checking, the way we should always exclude aliases from leak checking.

src/resolvers.ts

fnimick · 2024-01-02T14:53:25Z

@jeffijoe my conclusions from the above conversations:

aliasTo needs to be excluded from leak checking
value still needs to be leak checked, with its lifetime dependent upon the container to which it is registered.

Look at the koa example for a perfect illustration of the latter. https://github.com/jeffijoe/awilix/blob/master/examples/koa/index.js#L25
If messageService were registered as a singleton by mistake, it would leak the current user value out of the request. This absolutely needs to be caught by strict mode checks.

- Add `isLeakSafe` to resolver to support excluding alias registrations from lifetime leak checking (since their targets will be checked) - Add tests for alias registration lifetime leak checking - Add `ResolverInternal` interface to include optional `isLeakSafe` and `isValue` params, which should not be exposed in the public library interface

jeffijoe · 2024-01-02T15:56:12Z

src/types.ts

@@ -0,0 +1,16 @@
+import { Resolver } from './resolvers'
+
+export interface ResolverInternal<T> extends Resolver<T> {


Adding these to Resolver is ok, since each new property is optional; we don't need a new type. Besides, this "internal" type is leaking to the public since it's exposed in the public container interface. 😅

jeffijoe · 2024-01-02T16:01:21Z

src/types.ts

+  /**
+   * True if this resolver should be excluded from lifetime leak checking. Used to exclude alias
+   * resolvers since any lifetime issues will be caught in the resolution of the alias target.
+   */


Let's make this a bit more generic as well. It's perfectly fine for custom resolvers to use this. For example, I have an asLazy (not in core because I don't recommend it) that resolves when any method is called on the returned proxy.

Suggested change

/**

* True if this resolver should be excluded from lifetime leak checking. Used to exclude alias

* resolvers since any lifetime issues will be caught in the resolution of the alias target.

*/

/**

* True if this resolver should be excluded from lifetime leak checking. Used by resolvers

* that wish to uphold the anti-leakage contract themselves.

*/

jeffijoe · 2024-01-02T16:04:28Z

src/types.ts

+  /**
+   * True if this resolver is a value resolver. Used to implicit set the lifetime of a value
+   * resolver to either {@link Lifetime.SCOPED} or {@link Lifetime.SINGLETON} depending on whether
+   * this value is registered to a root or a scope container.
+   */
+  isValue?: boolean


I think we should make this a bit more generic, maybe autoLifetime? I'm trying to avoid coupling these new options to any particular resolver type.

Maybe as an alternative, we could use the root container to resolve any requested SINGLETON, that way any overrides would not be used by singletons, but could be used by scoped. Do you think that would have any negative side effects?

If we do this, then we can make asValue return lifetime: SCOPED, isLeakSafe: true and we'd only need the isLeakSafe option.

I considered that but changing the resolution behavior would be a breaking change outside of strict mode - unless you're suggesting that strict mode should change the resolution behavior?

If we were to do that, we could avoid leak checking for singletons entirely, since they will always resolve on the root container with only root container elements. Even a scoped registration set on the root container would be safe for a singleton to depend on because it will only ever pull that registration from the root container, and not from a potential scope.

The question is:

const container = createContainer({ strict: true }) container.register({ s: asFunction(cradle => cradle.value).singleton(), value: asValue('foo') }) const scope = container.createScope() scope.register({ value: asValue('bar') })

if you call scope.resolve('s), do you expect it in this case to return foo by resolving the scoped value from the root, or do you expect a lifetime leakage error since technically it's resolving the scoped value?

EDIT: To extend this hypothetical, would you expect a singleton resolving a scoped non-value module in the root scope to resolve successfully (though the module may fail to find its dependencies that are only registered in scopes) or would you expect it to error on a lifetime issue?

Actually @jeffijoe I just had an idea that sidesteps this problem entirely. For the purposes of lifetime checking, why not treat a scoped lifetime, defined at the root level, as equivalent to singleton if and only if the resolver used during resolution is that root-registered scoped module (rather than one defined in a scope)?

The only potential gotcha is that the error message for a "real" lifetime issue will be pushed down one level in the resolution stack. e.g. imagine A (singleton) depends on B (scoped, registered at root) depends on C (scoped, registered in scope). In the current setup, A -> B will trigger the error, but in this proposed change, it is only on the resolution of C that this problem is detected.

Regarding the first question, I would expect the result to be foo because the singleton would be using the registration from the root container; this seems like the most natural to me, as it means it doesn't matter whether it was resolved via a scope or the root, it would always be deterministic.

Now that you mention the case of a singleton depending on a scoped value, I currently don't see a legitimate reason for that ever happening with the exception of the leak-safe resolvers. Perhaps what we should do is have strict mode change the resolution behavior to the one we discussed in addition to enforcing these checks. I believe a singleton depending on non-singletons is the big issue we are trying to prevent with this change overall. By doing so, we'd get the (IMO) most natural behavior for asValue.

If this is the way we go with, then I would like to make strict the default in the next major; I would even want to remove the option entirely, as I think this is the behavior that everyone should be using.

Makes sense. I'll update this to the new behavior for strict and push.

@jeffijoe it'll be a bit. Changing the target container mid-resolution is a problem - i.e. if you resolve a scoped module, which has a dependency on a singleton, we want to switch resolution to the singleton for strict mode. Currently the assumption is that the container is always the same, and therefore closing over the resolution stack is sufficient since all resolve calls from dependencies will target the same container.

As soon as the container is switched due to the singleton in the resolution path, the resolution stack is effectively cleared, meaning that we go one further round before a cyclic dependency is detected and the resolution path indicating a possible error is truncated.

I am guessing you don't want to change the API to pass the resolution stack through the calls from containers to resolvers back to containers?

Yeah the resolution stack is an implementation detail so shouldn't be exposed. Hmm..

@jeffijoe done - I added a new internal container construction function to allow for sharing the same mutable resolution stack array between parents and scopes.

Should we remove the parentContainer argument from the public container constructor as well? Technically it's part of the existing public API but it was not documented and should not have been used. If people use that, it will again decouple the resolution stack between parent and child containers and lead to problems in strict mode.

Great work @fnimick ! 🙏

I agree, we should remove parentContainer. It was an implementation detail, I don't know why I didn't think of your solution earlier, I've used it plenty of times elsewhere. 😅

Remove `asValue` lifetime support, use `isLeakSafe` instead.

src/__tests__/container.test.ts

src/container.ts

jeffijoe · 2024-01-03T16:43:37Z

src/types.ts

@@ -0,0 +1,6 @@
+import { LifetimeType } from './lifetime'
+
+export type ResolutionStack = Array<{


We could probably put this type definition inside of container.ts, it feels a bit alone here. Alternatively, we could refactor the type definitions inside of container.ts to here, assuming we export them in index.ts. What do you think?

That makes sense. I only did this as a quick hack to remove ResolutionStack from the public API surface while avoiding having to manually specify all exports from ./container.ts (since the index has export *).

The proper solution is to go through and rather than using wildcard export, explicitly export only those items which are intended to be in the public API.

Would this type need to be exported if put in container.ts though?

Nope, I just went through and cleaned up all the wildcard exports to match what (I think) the public API should be.

Took a peek and I noticed RegistrationHash is a public interface but isn't exported. Could there be others?

fnimick · 2024-01-03T17:48:45Z

@jeffijoe now that this is a substantial change to the (undocumented) public API surface, do we want to keep it at 9.1 or move to 10.0?

jeffijoe · 2024-01-03T17:58:40Z

I would say the safest is to do a major to avoid any surprises.

fnimick · 2024-01-03T18:01:04Z

I'll update the docs to reference strict mode added in 10.0 - do we want to make it the default option?

jeffijoe · 2024-01-03T18:16:22Z

I think we should start with opt-in. As I mentioned before, I'd eventually want to not even have the option anymore, as I think strict mode should be used by everyone, but I'd like to get this into people's hands and get feedback first before making that move.

jeffijoe · 2024-01-03T18:24:30Z

@fnimick would you also like to do the honors of writing the changelog entry for v10? Traditionally I have always done it myself but this is a major contribution where having a paragraph or two about "this is what is changing, this is the motivation" would probably be written a lot better by the author than myself. 😄

I'm of course happy to do it, you've definitely done enough already! 🙏

fnimick · 2024-01-07T17:59:53Z

@jeffijoe is there anything else outstanding we need to do in order to merge and release?

jeffijoe · 2024-01-07T18:12:55Z

@fnimick sorry I didn't get notified about your latest commit, I'll merge this and release ASAP.

jeffijoe · 2024-01-07T18:52:55Z

@fnimick I had to fix the rollup config in #350 due to the change in the exports, but that has also been merged and I've released 10.0.0, thanks again!

fnimick added 2 commits December 28, 2023 17:04

Add additional test with injector proxy

30782b5

Confirms `errorOnShorterLivedDependencies` compatibility with per-module local injections.

fnimick added 2 commits December 28, 2023 17:50

Add support for lifetimes in asValue, default to scoped

ab47584

Correct comment re: scoped and parent cache

8cbdaa0

jeffijoe reviewed Dec 30, 2023

View reviewed changes

src/lifetime.ts Show resolved Hide resolved

src/resolvers.ts Outdated Show resolved Hide resolved

src/container.ts Show resolved Hide resolved

src/container.ts Outdated Show resolved Hide resolved

src/__tests__/lifetime.test.ts Show resolved Hide resolved

jeffijoe mentioned this pull request Dec 30, 2023

Is it possible to detect / error on singletons dependent on transient or scoped registrations? #348

Closed

Update per review feedback

ced24e7

jeffijoe reviewed Dec 30, 2023

View reviewed changes

README.md Show resolved Hide resolved

Add errorOnShorterLivedDependencies as recommended usage option

00f68ae

Also update all examples to use it (and fix examples where necessary)

fnimick added 3 commits January 1, 2024 14:41

Rename 'errorOnShorterLivedDependencies' to 'strict'

ddc89a7

Add documentation for strict mode

5ea979a

Add full strict mode support

787bf45

Update `asValue()` to automatically set lifetime based upon registration container (root vs scope) Add checks to throw an error when a singleton is registered on a scope in strict mode

jeffijoe reviewed Jan 2, 2024

View reviewed changes

fnimick added 2 commits January 2, 2024 10:24

Remove internal ResolutionStack from library external interface

31c4649

jeffijoe reviewed Jan 2, 2024

View reviewed changes

Update strict mode to only resolve singletons in root context

d8a2680

Remove `asValue` lifetime support, use `isLeakSafe` instead.

Add explicit test for parent/scope resolution stack sharing

f0bc058

jeffijoe reviewed Jan 3, 2024

View reviewed changes

fnimick added 2 commits January 3, 2024 12:34

Add explicit test for injector + definition in strict mode

d1af9b4

Clean up and limit public exports

bd38a1b

Update README

5fd2489

Add changelog entry and bump package to v10.0.0

6d1832b

jeffijoe merged commit 0186da9 into jeffijoe:master Jan 7, 2024
6 checks passed

radahh-rest mentioned this pull request Jan 10, 2024

Awilix 10 type exposure #352

Closed

		@@ -0,0 +1,16 @@
		import { Resolver } from './resolvers'

		export interface ResolverInternal<T> extends Resolver<T> {

		@@ -0,0 +1,6 @@
		import { LifetimeType } from './lifetime'

		export type ResolutionStack = Array<{

Add option to detect and error on dependency lifetime mismatch #349

Add option to detect and error on dependency lifetime mismatch #349

Conversation

fnimick commented Dec 28, 2023

fnimick commented Dec 28, 2023 • edited Loading

jeffijoe left a comment

Choose a reason for hiding this comment

coveralls commented Dec 30, 2023 • edited Loading

fnimick commented Dec 30, 2023

jeffijoe commented Dec 30, 2023

fnimick commented Dec 30, 2023

jeffijoe commented Dec 30, 2023

fnimick commented Dec 30, 2023 • edited Loading

jeffijoe commented Dec 30, 2023

fnimick commented Dec 31, 2023 • edited Loading

jeffijoe commented Dec 31, 2023 • edited Loading

fnimick commented Jan 1, 2024

fnimick commented Jan 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fnimick commented Jan 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fnimick Jan 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffijoe Jan 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fnimick Jan 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fnimick commented Jan 3, 2024

jeffijoe commented Jan 3, 2024

fnimick commented Jan 3, 2024

jeffijoe commented Jan 3, 2024

jeffijoe commented Jan 3, 2024

fnimick commented Jan 7, 2024

jeffijoe commented Jan 7, 2024

jeffijoe commented Jan 7, 2024

fnimick commented Dec 28, 2023 •

edited

Loading

coveralls commented Dec 30, 2023 •

edited

Loading

fnimick commented Dec 30, 2023 •

edited

Loading

fnimick commented Dec 31, 2023 •

edited

Loading

jeffijoe commented Dec 31, 2023 •

edited

Loading

fnimick commented Jan 1, 2024 •

edited

Loading

fnimick Jan 2, 2024 •

edited

Loading

jeffijoe Jan 2, 2024 •

edited

Loading

fnimick Jan 2, 2024 •

edited

Loading