CurtHagenlocher commented on code in PR #4075:
URL: https://github.com/apache/arrow-adbc/pull/4075#discussion_r3260660244
##########
csharp/src/Apache.Arrow.Adbc/DriverManager/ManagedDriverLoader.cs:
##########
@@ -69,27 +70,92 @@ namespace Apache.Arrow.Adbc.DriverManager
/// </item>
/// <item>
/// <description>
- /// Drivers should be published with their <c>.deps.json</c> file
alongside the
- /// assembly (the default when building with the .NET SDK). Native
dependencies
- /// should follow the standard <c>runtimes/<rid>/native</c>
layout.
+ /// Drivers <b>must</b> be published with their <c>.deps.json</c>
file alongside
+ /// the assembly (the default when building with the .NET SDK).
Without it the
+ /// loader cannot construct an <see
cref="AssemblyDependencyResolver"/>, the
+ /// host's TPA list is the only thing that gets consulted, and any
driver
+ /// dependency the host does not already have will fail to load at
the first
+ /// call site that touches it. Native dependencies should follow the
standard
+ /// <c>runtimes/<rid>/native</c> layout.
/// </description>
/// </item>
/// <item>
/// <description>
- /// If a driver does not ship a <c>.deps.json</c>, the loader still
works but
- /// falls back to whatever the host can resolve plus the driver's
directory.
- /// Callers needing strong isolation should load the driver into a
custom
- /// <see cref="AssemblyLoadContext"/> themselves.
+ /// A driver whose only dependencies are already present in the
host's TPA list
+ /// can technically load without a <c>.deps.json</c>, but this is
fragile and
+ /// not a supported configuration. Callers needing strong isolation
should
+ /// load the driver into a custom <see cref="AssemblyLoadContext"/>
themselves.
/// </description>
/// </item>
/// </list>
+ /// <para>
+ /// <b>Shared public contract assemblies and version skew:</b> a small set
of
+ /// assemblies form the public contract between the driver manager and any
managed
+ /// driver and must have <i>exactly one identity</i> per process. In
particular:
+ /// </para>
+ /// <list type="bullet">
+ /// <item><description><c>Apache.Arrow.Adbc</c> (defines <see
cref="AdbcDriver"/>, <c>AdbcDatabase</c>, etc.)</description></item>
+ /// <item><description><c>Apache.Arrow</c> (Arrow types crossing the API
boundary)</description></item>
+ /// <item><description><c>System.Diagnostics.DiagnosticSource</c>
(telemetry / Activity)</description></item>
+ /// </list>
+ /// <para>
+ /// Because the driver is loaded into the default <see
cref="AssemblyLoadContext"/>,
+ /// the runtime always returns the copy already loaded by the host for
these
+ /// assemblies; any version shipped next to the driver and listed in the
driver's
+ /// <c>.deps.json</c> is ignored. This is the desired behavior (type
identity is
+ /// preserved across the boundary) but it has an operational consequence:
+ /// <b>the host must ship versions of these contract assemblies that meet
or exceed
+ /// the minimum versions the driver was compiled against.</b> If the host
pins an
+ /// older version, the driver may throw <see
cref="MissingMethodException"/> or
+ /// <see cref="TypeLoadException"/> the first time it touches a newer API.
+ /// </para>
+ /// <para>
+ /// Private driver dependencies (i.e. assemblies the host does <i>not</i>
reference)
+ /// go through the per-driver <see cref="AssemblyDependencyResolver"/> and
load from
+ /// the driver's directory using its <c>.deps.json</c>. They are isolated
from host
+ /// versions of the same files only insofar as the host has not already
loaded them.
+ /// </para>
+ /// <para>
+ /// <b>Order-dependent resolution across multiple drivers:</b> if two
drivers ship
+ /// different versions of the same private dependency (for example
+ /// <c>Newtonsoft.Json</c> 12 in one driver and 13 in another), the first
driver
+ /// whose resolver satisfies that simple name "wins" for the lifetime of
the
+ /// process; the runtime will return the already-loaded copy for every
subsequent
+ /// request. When a later driver's resolver could also have satisfied the
same
+ /// simple name, the loader emits a warning via the
+ /// <c>Apache.Arrow.Adbc.DriverManager.ManagedDriverLoader</c>
+ /// <see cref="TraceSource"/> so the collision is debuggable instead of
silent.
+ /// Callers that need version isolation between drivers should load
conflicting
+ /// drivers into their own <see cref="AssemblyLoadContext"/> instances.
+ /// </para>
+ /// <para>
+ /// <b>Failed-load cleanup:</b> the per-driver <see
cref="AssemblyDependencyResolver"/>
+ /// is only published to the process-wide resolver cache (and the global
+ /// <see cref="AssemblyLoadContext.Resolving"/> hook is only armed) after
+ /// <see cref="Assembly.LoadFrom(string)"/> succeeds. If the driver
assembly itself
+ /// fails to load, no resolver is left behind to influence later assembly
+ /// resolutions. Once a driver loads successfully its resolver remains for
the
+ /// lifetime of the process; the default <see cref="AssemblyLoadContext"/>
is not
+ /// unloadable, so there is no safe point at which to remove it.
+ /// </para>
/// </remarks>
internal static class ManagedDriverLoader
{
#if NET6_0_OR_GREATER
private static readonly ConcurrentDictionary<string,
AssemblyDependencyResolver> resolvers
= new ConcurrentDictionary<string,
AssemblyDependencyResolver>(StringComparer.OrdinalIgnoreCase);
+ // Tracks which driver path "won" the very first contested resolution
for a
+ // given simple assembly name (e.g. "Newtonsoft.Json"). Used purely to
emit a
+ // diagnostic when a later resolver could also satisfy that name --
the runtime
+ // always returns the assembly already loaded by the first winner, so
any later
+ // candidate is silently shadowed.
Review Comment:
I'm not sure to what extent this is true under .NET Core. Two assemblies
with different load contexts can definitely resolve the same assembly to two
different files/versions. Is this code path always used or only if there's no
.deps file?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
