opensearch-project / opensearch-net

OpenSearch .NET Client
Apache License 2.0
105 stars 49 forks source link

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

Open andresanscartier opened 1 year ago

andresanscartier commented 1 year ago

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 methodDynamicCompositeResolver_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()

Xtansia commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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. https://github.com/elastic/elasticsearch-net/issues/4538

Do you still need a project example ?

Best regards

Xtansia commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

Best regards

Xtansia commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Hey @andresanscartier! Just following up here with an update. An issue has been opened for potentially fixing this in ILLink: https://github.com/dotnet/runtime/issues/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 commented 1 year ago

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.