Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Infinite recursion under certain configurations on Windows #722

Closed
1 of 5 tasks
kbolino opened this issue Oct 10, 2023 · 10 comments · Fixed by #730
Closed
1 of 5 tasks

Infinite recursion under certain configurations on Windows #722

kbolino opened this issue Oct 10, 2023 · 10 comments · Fixed by #730

Comments

@kbolino
Copy link
Contributor

kbolino commented Oct 10, 2023

Description

In certain recursive configuration scenarios on Windows only, there is infinite recursion in the (pkg/config).Config.Initialize method.

Offending configuration:

with-expecter: False
dir: foobar
packages:
  github.com/vektra/mockery/v2/pkg/fixtures/example_project/pkg_with_subpkgs/subpkg2:
    config:
      recursive: True
      with-expecter: True
      all: True
  github.com/vektra/mockery/v2/pkg/fixtures/example_project/pkg_with_subpkgs/subpkg2/subpkg3: {}

Discovered while fixing #718 in PR #720

Mockery Version

master @ 9d79f3a

Golang Version

> go version
go version go1.21.2 windows/amd64

Installation Method

  • Binary Distribution
  • Docker
  • brew
  • go install
  • Other: Source

Steps to Reproduce

  1. Be on Windows
  2. Checkout the aforementioned revision of the code
  3. Run the test TestConfig_Initialize/test_with_one_subpackage,_config_not_defined in package pkg/config
    • Also happens in TestConfig_Initialize/test_with_subpackage's_interfaces_defined
@kbolino
Copy link
Contributor Author

kbolino commented Oct 10, 2023

This is not hardly the only issue on Windows, which certainly complicates fixing it.

kbolino added a commit to kbolino/mockery that referenced this issue Oct 10, 2023
@LandonTClipp
Copy link
Collaborator

So I'm not sure I understand. What exactly is the infinite recursion here? What do you notice when running that config? (does mockery spin infinitely and not give any output?)

@LandonTClipp
Copy link
Collaborator

I admit, I have spent 0 mental energy making mockery work for Windows. It hasn't been a huge priority because most people use unix-like systems. I'm honestly surprised mockery works at all on windows TBH because there are a lot of underlying things like in pathlib (which I wrote) that don't have any explicit support for Windows, although maybe that's not as much of an issue if you're using WSL.

@kbolino
Copy link
Contributor Author

kbolino commented Oct 10, 2023

So I'm not sure I understand. What exactly is the infinite recursion here? What do you notice when running that config? (does mockery spin infinitely and not give any output?)

It's a little difficult to get a good working output because the YAML encoder seems to run out of memory before it can print anything. Inferring from what I can see from spew.Dump, what seems to happen is that the configuration gets turned into something like the following (ignoring the parts that aren't broken):

packages:
  github.com/vektra/mockery/v2/pkg/fixtures/example_project/pkg_with_subpkgs/subpkg2/subpkg3:
    packages:
      github.com/vektra/mockery/v2/pkg/fixtures/example_project/pkg_with_subpkgs/subpkg2\subpkg3:
        packages:
          github.com/vektra/mockery/v2/pkg/fixtures/example_project/pkg_with_subpkgs/subpkg2/subpkg3:
            packages:
              github.com/vektra/mockery/v2/pkg/fixtures/example_project/pkg_with_subpkgs/subpkg2\subpkg3:
                ...

@LandonTClipp
Copy link
Collaborator

Oh, so you are saying that showconfig is getting into an infinite loop, not the actual mock generation?

@kbolino
Copy link
Contributor Author

kbolino commented Oct 10, 2023

I haven't tried generating mocks using the problematic configuration yet, so I don't know if mock generation is broken or not. I can test that later when I'm back on Windows. So far, this only arose for me in mockery's own tests.

@kbolino
Copy link
Contributor Author

kbolino commented Oct 10, 2023

I admit, I have spent 0 mental energy making mockery work for Windows. It hasn't been a huge priority because most people use unix-like systems. I'm honestly surprised mockery works at all on windows TBH because there are a lot of underlying things like in pathlib (which I wrote) that don't have any explicit support for Windows, although maybe that's not as much of an issue if you're using WSL.

That's fair, it may not be worth the time or effort if nobody really uses it on Windows. If the right answer is just to use Mac/WSL/Linux instead, maybe the right solution to this is just to add a note to the README that Windows isn't supported and move on.

@LandonTClipp
Copy link
Collaborator

LandonTClipp commented Oct 10, 2023

Got it, so I think that this config would work fine for actual mock generation. showconfig has been broken since I implemented the recursive feature, I just never mentioned it and it seemed like no one actually ever used showconfig until now. I'm also not sure this has anything to do with Windows necessarily.

The problem lies with the fact that when using recursive: True, all discovered sub-packages will contain a copy of the parent package's config, including the package parameter itself. Because of this copying behavior, the package parameter gets into this infinite self-referential loop that is impossible to get out of. This isn't a problem for the mock generation because it doesn't recurse into the packages map, it only looks at the top-level packages map.

I did try fixing this like a year ago but obviously I gave up... I think the solution would be to nil out the packages map during the config inheritance step. It's not a great solution but I think it'd work.

@kbolino
Copy link
Contributor Author

kbolino commented Oct 10, 2023

There does seem to be something especially bad about it on Windows, where the path separator discrepancy (\ for the file system, / for Go packages) creates redundant entries in the packages map which I guess breaks the YAML encoder's cycle detection.

@LandonTClipp
Copy link
Collaborator

FWIW, my latest patch (that closed this issue) fixes the behavior of showconfig so that we don't get into infinite recursion when using recursive: True

➜  mockery git:(master) ✗ ./mockery showconfig
05 Nov 23 22:23 CST INF Using config: /Users/landonclipp/git/vektra/mockery/.mockery.yaml version=v0.0.0-dev
disable-version-string: true
filename: '{{.MockName}}.go'
mockname: '{{.InterfaceName}}'
outpkg: mocks
packages:
  github.com/vektra/mockery/v2/pkg:
    config:
.....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants