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

[BUG] Internal Utf8Json is incompatible with ILLink trimming #370

Open
andresanscartier opened this issue Sep 20, 2023 · 11 comments
Open

[BUG] Internal Utf8Json is incompatible with ILLink trimming #370

andresanscartier opened this issue Sep 20, 2023 · 11 comments
Labels
bug Something isn't working

Comments

@andresanscartier
Copy link

andresanscartier commented Sep 20, 2023

What is the bug?

With IOS nuget OpenSearch.CLient 1.5.0 the "new ConnectionSettings(uri)" method crash with MethodAccessException error.

How can one reproduce the bug?

Try ConnectionSettings with (Uri uri = null, IConnection connection = null) signature.

What is your host/environment?

JetBrains Rider 2023.2.1
Build #RD-232.9559.61, built on August 22, 2023
Runtime version: 17.0.8+7-b1000.8 x86_64
VM: OpenJDK 64-Bit Server VM by JetBrains s.r.o.
macOS 13.0.1
.NET Core v7.0.7 x64 (Server GC)

IOS IPad 16.3.1

Do you have any additional context?

This code works well on Windows, Mac, Android and Linux but crash at the new ConnectionSettings on IOS.

        try
        {
            ConnectionSettings connectionSettings = new ConnectionSettings(new Uri(this.configuration.ApiUrl));
            connectionSettings.BasicAuthentication(this.configuration.Username, this.configuration.Password);
            this.client = new OpenSearchClient(connectionSettings);
        }
        catch (Exception e)
        {
            this.client = null;
        }

With this error:
System.MethodAccessException: Method OpenSearch.Net.Utf8Json.Resolvers.DynamicCompositeResolver..ctor(OpenSearch.Net.Utf8Json.IJsonFormatter[],OpenSearch.Net.Utf8Json.IJsonFormatterResolver[])' is inaccessible from method DynamicCompositeResolver_868f210720364292b1ea67d7111b98de..ctor(OpenSearch.Net.Utf8Json.IJsonFormatter[],OpenSearch.Net.Utf8Json.IJsonFormatterResolver[])'
at DynamicCompositeResolver_868f210720364292b1ea67d7111b98de..ctor(IJsonFormatter[] , IJsonFormatterResolver[] )
at System.Reflection.RuntimeConstructorInfo.InternalInvoke(Object , Object[] , Boolean )
at System.Reflection.RuntimeConstructorInfo.DoInvoke(Object , BindingFlags , Binder , Object[] , CultureInfo )
at System.Reflection.RuntimeConstructorInfo.Invoke(BindingFlags , Binder , Object[] , CultureInfo )
at System.RuntimeType.CreateInstanceImpl(BindingFlags , Binder , Object[] , CultureInfo )
at System.Activator.CreateInstance(Type type, BindingFlags bindingAttr, Binder binder, Object[] args, CultureInfo culture, Object[] activationAttributes)
at System.Activator.CreateInstance(Type type, Object[] args)
at OpenSearch.Net.Utf8Json.Resolvers.DynamicCompositeResolver.Create(IJsonFormatter[] , IJsonFormatterResolver[] )
at OpenSearch.Client.OpenSearchClientFormatterResolver.InnerResolver..cctor()

@andresanscartier andresanscartier added bug Something isn't working untriaged labels Sep 20, 2023
@Xtansia Xtansia removed the untriaged label Sep 20, 2023
@Xtansia
Copy link
Collaborator

Xtansia commented Sep 21, 2023

@andresanscartier Very interesting that this is iOS specific, I haven't done any development on .NET targeting iOS yet, I'll try to have a go at reproducing this.

In the meantime if you're able to share a minimal reproducing project that'd be great, and is this reproducible inside a device emulator or only on hardware?

@andresanscartier
Copy link
Author

@Xtansia
This appears to be a Linker issue.
In a small, unlinked project, this doesn't crash.
We are trying to figure out what to put in the Preserve.xml file.
"OpenSearch.Net" and "OpenSearch.Client" don't seem to be enough.
An idea ?
Thanks!

@andresanscartier
Copy link
Author

andresanscartier commented Sep 21, 2023

@Xtansia
Finally, we need to find a way to use your library in an iOS project with link behavior set to "Link Everything" using Preserve.xml file.

Not sure if this is related, but ElasticSearch seems to have had a similar issue with its Net.Utf8Json.Resolvers.DynamicCompositeResolver_??? which was also constantly changing name and causing bad behaviors.

Here is the link to this issue.
elastic/elasticsearch-net#4538

Do you still need a project example ?

Best regards

@Xtansia
Copy link
Collaborator

Xtansia commented Sep 21, 2023

@andresanscartier The issue linked appears to be a different issue and relates to their renaming of assemblies and is from before OpenSearch's fork so is in the history of this repo.

Your note about "Link Everthing" being the issue has pointed me in the right direction and I'm now able to repro, I have some suspicions about what's happening so will do some more digging and then return with my findings

@Xtansia
Copy link
Collaborator

Xtansia commented Sep 22, 2023

So the issue seems to be that even you flag the OpenSearch.Net assembly as being preserved, ILLink still trims the InternalsVisibleTo attributes which are necessary due to the dynamic assemblies generated by Utf8Json referencing internal details.

I'm still investigating, but have not yet found any solution other than turning down the link level, or modifying the library to make the internals of Utf8Json public which would not be ideal.

@andresanscartier
Copy link
Author

@Xtansia
Thanks for your follow up.
We're glad you can reproduce our issue.

Best regards

@Xtansia
Copy link
Collaborator

Xtansia commented Sep 24, 2023

I asked in the discord channel for .NET linker/ILLink, and unfortunately looks like there's currently no way to stop this behaviour without reducing the link mode to off/sdk-only, performing some terrible work-arounds/hacks (e.g adding empty assemblies at link time then removing, or patching IVT attributes back into the assembly after linking), or making modifications to this library to make the internals public instead.

Screenshot 2023-09-25 at 12 03 26 PM

@andresanscartier
Copy link
Author

Hi @Xtansia
We will conduct some tests to modify the link behavior, but in the past, we changed those settings because our app wouldn't work otherwise. You mentioned, 'modifying the library to make the internals of Utf8Json public, which would not be ideal.' I wonder why?
Would it potentially cause any other adverse side effects in case we decide to go ahead with making that change in our own fork?

@Xtansia
Copy link
Collaborator

Xtansia commented Sep 25, 2023

Hi @Xtansia We will conduct some tests to modify the link behavior, but in the past, we changed those settings because our app wouldn't work otherwise. You mentioned, 'modifying the library to make the internals of Utf8Json public, which would not be ideal.' I wonder why? Would it potentially cause any other adverse side effects in case we decide to go ahead with making that change in our own fork?

My reasoning isn't about any adverse side effects, my concerns are more around if we made such a modification in the library proper, Utf8Json now becomes part of the library's public API surface and so has implications around support for end-users interacting with it along with semver commitments

@Xtansia
Copy link
Collaborator

Xtansia commented Sep 27, 2023

Hey @andresanscartier! Just following up here with an update.
An issue has been opened for potentially fixing this in ILLink: dotnet/runtime#92582

In the meantime I've managed to create an MSBuild task you could use to work around this, basically just lets you specify assemblies to have their InternalsVisibleTo attributes copied back into after ILLink trimming: https://github.com/Xtansia/IvtPatch & https://www.nuget.org/packages/Xtansia.IvtPatch

You'd basically just the below into your csproj:

<Project>
    <!-- ... SNIP ... -->

    <ItemGroup>
        <PackageReference Include="Xtansia.IvtPatch" Version="1.0.0" />
    </ItemGroup>
    
    <ItemGroup>
        <IvtPatchAssembly Include="OpenSearch.Client" />
        <IvtPatchAssembly Include="OpenSearch.Net" />
    </ItemGroup>
</Project>

And then add then make sure your Preserve.xml/TrimmerRootDescriptor contains:

<linker>
    <assembly fullname="OpenSearch.Net" preserve="all" />
    <assembly fullname="OpenSearch.Client" preserve="all" />
</linker>

@andresanscartier
Copy link
Author

Hi @Xtansia,
Sorry for the delay. Thank you very much! Your temporary solution seems to be working. At the moment, I am unable to access our OpenSearch Dashboard, but when I trace the code, I can see that nothing crashes when sending the statistics.
It is greatly appreciated.

@Xtansia Xtansia changed the title [BUG] new ConnectionSettings(uri) Crash on IOS [BUG] Internal Utf8Json is incompatible with ILLink trimming Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants