Regression: secondary HasManyThrough inclusion fails with circular dependency

[0x0aNL]

After the latest release (and possibly the one before - hard to test), a circular dependency error triggers where none did before.

Steps to reproduce

1. Create 3 models, e.g. User, Participant, Conversation
2. Set up bi-directional HasManyThrough relations (User through Participant to Conversation, Conversation through Participant to User)
3. Use one repository to find instances of the other and include the first, e.g. userRepo.conversations(userId).find({include:['users']});
4. Execute create: userRepo.conversations(userId).create({});
5. Execute find again: userRepo.conversations(userId).find({include:['users']});

Current Behavior

The first execution of find succeeds. The second fails with (using Todo <-> Assignment <-> Person from linked branch):

Error: Circular dependency detected: repositories.TodoRepository --> @TodoRepository.constructor[2] --> repositories.PersonRepository --> @PersonRepository.constructor[2] --> repositories.TodoRepository
      at ResolutionSession.pushBinding (/home/X/project/loopback-next/packages/context/src/resolution-session.ts:228:13)
      at Function.runWithBinding (/home/X/project/loopback-next/packages/context/src/resolution-session.ts:107:13)
      at Binding.getValue (/home/X/project/loopback-next/packages/context/src/binding.ts:535:40)
      at TodoListApplication.getValueOrPromise (/home/X/project/loopback-next/packages/context/src/context.ts:912:32)
      at TodoListApplication.get (/home/X/project/loopback-next/packages/context/src/context.ts:712:17)
      at getter (/home/X/project/loopback-next/packages/context/src/inject.ts:444:16)
      at fetchHasManyThroughModels (/home/X/project/loopback-next/packages/repository/src/relations/has-many/has-many-through.inclusion-resolver.ts:88:30)
      at processTicksAndRejections (node:internal/process/task_queues:96:5)
      at /home/X/project/loopback-next/packages/repository/src/relations/relation.helpers.ts:112:21
      at async Promise.all (index 0)
      at Object.includeRelatedModels (/home/X/project/loopback-next/packages/repository/src/relations/relation.helpers.ts:120:3)

Expected Behavior

Second execution of find on the HasManyThroughRepositoryFactory should succeed as well.

Link to reproduction sandbox

The Todo example at https://github.com/0x0aNL/loopback-next/tree/hmt-circdep-regression is set up with bi-directional HasManyThrough relations: Todo <-> Assignment <-> Person. A test case in src/__tests__/acceptance/todo.acceptance.ts executes:

await todoRepo.people(1).find({include: ['todos']});
await todoRepo.people(1).create({});
await todoRepo.people(1).find({include: ['todos']});

Additional information

> node -e 'console.log(process.platform, process.arch, process.versions.node)'
linux x64 16.1.0

> npm ls --prod --depth 0 | grep loopback
@loopback/example-todo@3.11.1 /home/X/project/loopback-next/examples/todo
├── @loopback/boot@3.4.1
├── @loopback/core@2.16.1
├── @loopback/repository@3.7.0
├── @loopback/rest-explorer@3.3.1
├── @loopback/rest@9.3.1
├── @loopback/service-proxy@3.2.1
├── loopback-connector-rest@4.0.1

It looks as though this regression is introduced between @loopback/repository@3.5.0 / @loopback/core@2.15.0 and @loopback/repository@3.6.0 / @loopback/core@2.16.0.

[raymondfeng]

@thephoenixofthevoid Can you check if this is related to your PRs in @loopback/context?

https://github.com/strongloop/loopback-next/pull/7527

[thephoenixofthevoid]

I will try to test with the previous release first. Because if it fails at the previous one, then it's unrelated to the changes in PR.

[thephoenixofthevoid]

Doesn't look like the mentioned commit is to blame. I may investigate this issue if you want to. And then report as a comment below.

[loopback-next] $ git checkout hmt-circdep-regression
[loopback-next] $ git checkout -b test
[loopback-next] $ git revert e2a0c4fecc6417eab260d6d9b725e87ed90b50b0
[test a5a44cfc0] feat(context): revert simplify resolutionsession by inlining enterInjection and enterBinding internals
 1 file changed, 50 insertions(+), 20 deletions(-)    
[loopback-next] $ cd examples/todo
[todo] $ yarn test
yarn run v1.22.10
warning ../../../../package.json: No license field
$ npm run rebuild

> @loopback/example-todo@3.11.1 rebuild /home/feanor/Documents/loopback-next/examples/todo
> npm run clean && npm run build

> @loopback/example-todo@3.11.1 clean /home/feanor/Documents/loopback-next/examples/todo
> lb-clean *example-todo*.tgz dist *.tsbuildinfo package

> @loopback/example-todo@3.11.1 build /home/feanor/Documents/loopback-next/examples/todo
> lb-tsc

$ lb-mocha "dist/__tests__/**/*.js"

  ..!....,,...................,.........

  34 passing (11s)
  3 pending
  1 failing

  1) TodoApplication
       creates & fetches related models:
     Error: Circular dependency detected: repositories.TodoRepository --> @TodoRepository.constructor[2] --> repositories.PersonRepository --> @PersonRepository.constructor[2] --> repositories.TodoRepository
      at ResolutionSession.pushBinding (/home/feanor/Documents/loopback-next/packages/context/src/resolution-session.ts:258:13)
      at Function.enterBinding (/home/feanor/Documents/loopback-next/packages/context/src/resolution-session.ts:105:13)
      at Function.runWithBinding (/home/feanor/Documents/loopback-next/packages/context/src/resolution-session.ts:120:49)
      at Binding.getValue (/home/feanor/Documents/loopback-next/packages/context/src/binding.ts:535:40)
      at TodoListApplication.getValueOrPromise (/home/feanor/Documents/loopback-next/packages/context/src/context.ts:912:32)
      at TodoListApplication.get (/home/feanor/Documents/loopback-next/packages/context/src/context.ts:712:17)
      at getter (/home/feanor/Documents/loopback-next/packages/context/src/inject.ts:444:16)
      at fetchHasManyThroughModels (/home/feanor/Documents/loopback-next/packages/repository/src/relations/has-many/has-many-through.inclusion-resolver.ts:88:30)
      at processTicksAndRejections (internal/process/task_queues.js:95:5)
      at runNextTicks (internal/process/task_queues.js:64:3)
      at processImmediate (internal/timers.js:435:9)
      at /home/feanor/Documents/loopback-next/packages/repository/src/relations/relation.helpers.ts:112:21
      at async Promise.all (index 0)
      at Object.includeRelatedModels (/home/feanor/Documents/loopback-next/packages/repository/src/relations/relation.helpers.ts:120:3)

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

[thephoenixofthevoid]

I commented out the whole checking for Circular dependency and it worked.

I think we should apply the following logic (which is used in inversify):

1. try ... catch for an error in runWithBinding
2. If it's the stackoverflow error, throw our error with information about bindings
3. Otherwise rethrow

It seems like there are changes in other packages that made 2 bindings referentially equal, thus triggering logic here:

    if (this.stack.find(i => isBinding(i) && i.value === binding)) {
      const msg =
        `Circular dependency detected: ` +
        `${this.getResolutionPath()} --> ${binding.key}`;
      debugSession(msg);
      throw new Error(msg);
    }

which is appeared 2+ years ago. And it seems it is rather working correctly.

The basic idea here is that circular dependency is not always evil, especially when they do not trigger recursive execution. In the latter case we will get error anyway. So catching it is a good way to not throw in good cases.

[thephoenixofthevoid]

$ git checkout d75d79d568b19fa632696e032e136e7e0253e85a
[Manually copy todo into examples]
$ cd examples/todo
$ yarn run test
// Same error here

(yarn run clean && yarn run build is done too)

Link to the state: d75d79d568b19fa632696e032e136e7e0253e85a The bug is much older than 0x0aNL suggested.

[thephoenixofthevoid]

Originally taken from inversify:

export function isCallStackOverflowError(error: Error) {
  if (error.message === "Maximum call stack size exceeded") {
    return true;
  }
  if (error instanceof RangeError) {
    return true;
  }
  return false;
}

[0x0aNL]

Thanks for getting the investigation on so rapidly and thoroughly!

The bug is much older than 0x0aNL suggested.

I ran into this bug while upgrading the API I'm working on from aforementioned @loopback/repository@3.5.0 and @loopback/core@2.15.0. It took me a lot of time to reproduce it with a minimal framework, and at that time I thought I had found the issue. However, the code to "reproduce" the bug clearly is not the exact same, because as you point out it will not work with older versions either. Apologies for the confusion!

The question I'm trying to answer for my case is: is the root cause of the bug the same, if our API clearly breaks by upgrading while the reproduction repo does not? Unfortunately I cannot share the code of our API, but I'm determined to find out what exactly the difference is, or in other words, why I can run find/create/find in our API without issue.

Again thanks for the great work, I hope to get back with answers soon!

[thephoenixofthevoid]

Could you please share some more information about your API that you try to upgrade. What was the error message? Does removing the check for circular dependency helps?

That is, put aside the reproduction repo, what was the exact error you faced?

[0x0aNL]

The error we encounter when upgrading the API is:

     Error: Circular dependency detected: repositories.UserRepository --> @UserRepository.constructor[16] --> repositories.ConversationRepository --> @ConversationRepository.constructor[1] --> repositories.UserRepository
      at ResolutionSession.pushBinding (node_modules/@loopback/core/node_modules/@loopback/context/src/resolution-session.ts:228:13)
      at Binding.getValueOrProxy (node_modules/@loopback/core/node_modules/@loopback/context/src/binding.ts:576:13)
      at Binding.getValue (node_modules/@loopback/core/node_modules/@loopback/context/src/binding.ts:521:23)
      at SlipApi.getValueOrPromise (node_modules/@loopback/core/node_modules/@loopback/context/src/context.ts:912:32)
      at SlipApi.get (node_modules/@loopback/core/node_modules/@loopback/context/src/context.ts:712:17)
      at getter (node_modules/@loopback/core/node_modules/@loopback/context/src/inject.ts:444:16)
      at fetchHasManyThroughModels (node_modules/@loopback/repository/src/relations/has-many/has-many-through.inclusion-resolver.ts:88:30)
      at processTicksAndRejections (node:internal/process/task_queues:96:5)
      at /usr/src/app/node_modules/@loopback/repository/src/relations/relation.helpers.ts:112:21
      at async Promise.all (index 0)
      at Object.includeRelatedModels (node_modules/@loopback/repository/src/relations/relation.helpers.ts:120:3)

In this setup, there's a bi-directional HasManyThrough relation between User and Conversation through ConversationUser, and the triggering code (now part of an acceptance test) is:

      await userRepo.conversations(1).find({include: ['users']});
      await userRepo.conversations(1).create({});
      await userRepo.conversations(1).find({include: ['users']});

Interestingly, in order to trigger this error again so I could paste it here, I just installed @loopback/core@2.16.0. Comparing the sets of packages using npm ls --prod --depth 0 | grep loopback:

Error:

+-- @loopback/authentication-jwt@0.8.0
+-- @loopback/authentication@7.1.0
+-- @loopback/authorization@0.8.0
+-- @loopback/boot@3.3.0
+-- @loopback/build@6.3.0
+-- @loopback/context@3.15.0
+-- @loopback/core@2.16.0
+-- @loopback/cron@0.4.0
+-- @loopback/express@3.2.0
+-- @loopback/health@0.7.0
+-- @loopback/metrics@0.7.0
+-- @loopback/model-api-builder@2.2.0
+-- @loopback/openapi-v3@5.2.0
+-- @loopback/repository-json-schema@3.3.0
+-- @loopback/repository@3.5.0
+-- @loopback/rest-explorer@3.2.0
+-- @loopback/rest@9.2.0
+-- @loopback/security@0.4.0
+-- @loopback/service-proxy@3.1.0
+-- @loopback/testlab@3.4.1
+-- loopback-connector-mysql@6.0.1

No error:

+-- @loopback/authentication-jwt@0.8.0
+-- @loopback/authentication@7.1.0
+-- @loopback/authorization@0.8.0
+-- @loopback/boot@3.3.0
+-- @loopback/build@6.3.0
+-- @loopback/context@3.15.0
+-- @loopback/core@2.15.0
+-- @loopback/cron@0.4.0
+-- @loopback/express@3.2.0
+-- @loopback/health@0.7.0
+-- @loopback/metrics@0.7.0
+-- @loopback/model-api-builder@2.2.0
+-- @loopback/openapi-v3@5.2.0
+-- @loopback/repository-json-schema@3.3.0
+-- @loopback/repository@3.5.0
+-- @loopback/rest-explorer@3.2.0
+-- @loopback/rest@9.2.0
+-- @loopback/security@0.4.0
+-- @loopback/service-proxy@3.1.0
+-- @loopback/testlab@3.4.1
+-- loopback-connector-mysql@6.0.1

Comparing the package-lock.json between the two also shows (as with npm ls with higher depth) that @loopback/context@3.17.0 is installed as a non-deduped dependency of @loopback/core@2.16.1.

ETA: We're using Docker with a separate node_modules volume, which makes it a bit difficult to remove the check for circular dependency, but I'm going to give it a shot.

[thephoenixofthevoid]

Do you mix components from different releases?

Also, I need to check how core versions differ.

What is for sure, the change responsible for it happened before #7527. That is good to know

[thephoenixofthevoid]

Well, it seems you are having conflicting versions of context loaded at the same time. Try to update to newer version at the same time for all components

[0x0aNL]

Do you mix components from different releases?

Also, I need to check how core versions differ.

What is for sure, the change responsible for it happened before #7527. That is good to know

We update components once in a while (my preference is every release), and when we do, we upgrade all components at the same time. Unfortunately that's where things went haywire this time around, which is why the component lists are a bit of weird mix right now. That leads me to a somewhat related question: there is an upgrade tool, but what is the best way to downgrade to a consistent set of components / to upgrade to a set that is not the most recent?

[thephoenixofthevoid]

Well, https://loopback.io/doc/en/lb4/Update-generator.html Is that what you are looking for?

[0x0aNL]

Unless I'm missing something in the documentation, it's not possible to tell the update generator to create a set compatible with a given component, e.g. @loopback/core@2.15.0. Downgrading @loopback/cli before-hand also does not seem to lead to a downgrade in packages.json.

[0x0aNL]

Does removing the check for circular dependency helps?

Commenting out the throw new Error(msg); in pushBinding indeed works just fine!

[thephoenixofthevoid]

@raymondfeng, create a PR that will defer circular dependency error until actual stack overflow ocurrence? Long story short: this case is when the reappearance of same binding down the resolution tree is normal and doesn't lead to overthrow.

[thephoenixofthevoid]

@0x0aNL consider commenting out the whole if block (as shown above). Without throw it is just wasting CPU time anyway. Also note that in case of actual stackoverflow, you WILL receive error, but not the fancy one with resolution path. I think it's likely an acceptable option.

[0x0aNL]

I finally found the difference between the repro-repo and our API, that made our API work with @loopback/context@3.15.0. Our UserRepository is a singleton!

I added some commits to https://github.com/0x0aNL/loopback-next/tree/hmt-circdep-regression. The last commit should trigger the circular dependency error, while the one before with @loopback/context@3.15.0 instead of @loopback/context@3.16.0 doesn't.

I suppose this does make it very edge-casey - a bi-directional HasManyThrough relation with subsequent find/create/find whilst one of the source repositories is a singleton. Nevertheless I'm glad to have found the one factor leading to a difference in behaviour - I was beginning to doubt my sanity!

[achrinza]

create a PR that will defer circular dependency error until actual stack overflow ocurrence?

Perhaps we should also consider tweaking the existing setup as an informational alert as well? Otherwise, this may be a bottleneck that may be difficult to debug (i.e. not all circular dependencies are deliberate).

[thephoenixofthevoid]

It will become a problem once we have a truly async resolution. In meanwhile it's ok to simply catch stack overflow. The reason is that you have to do something to prevent recursive execution, it very unlikely to do it by mistake and it is very fragile thing that needs special care.

[thephoenixofthevoid]

In case of async resolution we will have to fibers as a call stack to track the dependencies between contexts. Because of that the depth of call stack won't ever be more that ~20 and we will stuck in infinite execution actually never getting an error. So I would rather think of it as of a future problem. And fundamental problem, by the way.

[mgabeler-lee-6rs]

I'm running into what I think is this same issue -- code that was previously working with various Repository classes referencing each other for inclusion resolvers via @repository.getter was working fine in the past. I even recall reading some LB documentation saying that @inject.getter and its friends were specifically meant for avoiding circular dependencies like this. But now with the latest release versions, this is failing.

Similar to 0x0aNL, I found that decorating the repositories with @injectable({ scope: BindingScope.SINGLETON }) seems to work around the issue.

Are there any known downsides or risks to making the repo classes singletons? Things I can think of that don't apply to me:

• You might re-use the same repo class in an app with different datasources
• You might be making per-request changes to the repo's configured behavior (reaching in and adding observers or other hooks)

[achrinza]

@mgabeler-lee-6rs There shouldn't be any issues with using Singletons for basic use-cases.