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

Detect circular dependencies among resources #714

Merged
merged 10 commits into from
Aug 16, 2019

Conversation

cafour
Copy link
Contributor

@cafour cafour commented Jul 11, 2019

Fixes #710.

The first commit merely satisfies my OCD. It's the second one that needs attention.

Make solely cosmetic changes to the class.
Copy link
Member

@exyi exyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first commit merely satisfies my OCD. It's the second one that needs attention.

Please don't, the same explanation as in the other PR applies - it does not really do anything for readability and pollutes history.

Anyways, the check works, but only with a the assumption that the resource dependencies are not liable, which is unfortunately not the case. I think we should keep this check you just implemented and add a second one to the point when the resources are ordered in the render phase.

@cafour
Copy link
Contributor Author

cafour commented Jul 29, 2019

@exyi History unpolluted.

There's something I don't understand though. How exactly can it break in the render phase if the user is not able to register resources with circular dependencies? Wouldn't the check slow down the render phase?

@exyi
Copy link
Member

exyi commented Jul 29, 2019 via email

@tomasherceg
Copy link
Member

I think that there would be many cases like that because many things in the configuration is mutable.
I think that we can live with that - if someone changes the configuration after startup, they deserve the exception.

@cafour
Copy link
Contributor Author

cafour commented Aug 16, 2019

I added a check to the point where resources are ordered and to the ResourceManager.AddRequiredResource method since both caused StackOverflowExceptions when the user tampered with the dependencies after registration.

and use HashSet<IResource> instead
@cafour cafour merged commit 0a54ed0 into master Aug 16, 2019
@cafour cafour deleted the fix/resources-circular-dependency branch August 16, 2019 15:54
@cafour cafour restored the fix/resources-circular-dependency branch August 16, 2019 15:57
@cafour cafour deleted the fix/resources-circular-dependency branch August 16, 2019 15:57
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 this pull request may close these issues.

Resources - Circular dependency - StackOverflow exception
4 participants