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

fix: Lazy loading of type libraries in case of type is class #330

Merged
merged 11 commits into from
Jan 10, 2025

Conversation

mavToday
Copy link
Contributor

@mavToday mavToday commented Jan 8, 2025

with commit #326 you introduced a lazy loading of type libraries.

the problem is, that this works only if there is a request of interfaces, but not of classes. I got a problem, that a method in assembly b could not resolve a class-type of assembly a, because the type library was not loaded yet.

In TypeProvider.GetVarType it resulted in a VT_UNKNOWN. This issues is only present with the changes of the given commit.

@mavToday
Copy link
Contributor Author

mavToday commented Jan 8, 2025

I added some tests.

TypeLibTest.TypeLib_ShouldBeLoaded_By_Class failes before my changes, because the classtype is first requested.

@marklechtermann marklechtermann changed the title Fixed lazy loading of type libraries fix: Lazy loading of type libraries Jan 8, 2025
src/dscom/TypeInfoResolver.cs Outdated Show resolved Hide resolved
src/dscom/TypeInfoResolver.cs Outdated Show resolved Hide resolved
@mavToday
Copy link
Contributor Author

mavToday commented Jan 9, 2025

Okay, this change give us several issues with the unit-tests, because it loads the type-libs perhaps several times.

EDIT:
It tried to load typelibs for object, string, func`1, delegate and so on. And load typelibs than it should does. I now skipp the built in types and it works.

@marklechtermann marklechtermann changed the title fix: Lazy loading of type libraries fix: Lazy loading of type libraries in case of type is class Jan 9, 2025
@mavToday
Copy link
Contributor Author

mavToday commented Jan 9, 2025

can you restart the Code Style Check? looks like a bug in the pipeline

@marklechtermann
Copy link
Member

can you restart the Code Style Check? looks like a bug in the pipeline

Yes, it's an error in the pipeline. I don't know why, but sometimes the pipeline doesn't find the .net runtime.

@marklechtermann
Copy link
Member

Okay, this change give us several issues with the unit-tests, because it loads the type-libs perhaps several times.

EDIT: It tried to load typelibs for object, string, func`1, delegate and so on. And load typelibs than it should does. I now skipp the built in types and it works.

i have changed the behavior of the some test base classes. See #332
I hope this will stop (or at least reduce) the sporadic errors during test execution.
We now have so many tests that we end up with dead locks, especially when executing them in parallel.

@mavToday
Copy link
Contributor Author

mavToday commented Jan 10, 2025

I updated the branch from main and adapted some changes.

BaseTest: There is no need anymore to create the dynamic-folder. Therefore i removed the code and the creation of unique assemblynames, because this was only needed for the file system.

DynamicAssemblyBuilder + TypeLibTest + TypeLibExporterNotifySink: I added the feature to add dependencies, because the typeresolver could not resolve them by the filesytem.

RegistrationServiceTest + HResult: I changed to assert to HResult, because this is not dependening on location. My system is german and it will create german messages (happend to me in net4.8 and changing the CultureInfo does no fullfill the job).

GlobalUsing: dotnet format --verify-no-changes

Thanks for the support

@marklechtermann
Copy link
Member

@mlessmann can you take a look at our product to see if the changes have negative side effects.
This affects the TypeInfoResolver implementation that you last modified.

Copy link
Contributor

@mlessmann mlessmann left a comment

Choose a reason for hiding this comment

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

LGTM

@matthiasnissen
Copy link
Member

Squash and merge...

@matthiasnissen matthiasnissen merged commit 8a5640b into dspace-group:main Jan 10, 2025
5 checks passed
@marklechtermann
Copy link
Member

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.

4 participants