AssemblyLoadContext.LoadFromStream relys on Length being available

[MaxwellDAssistek]

Description

When using AssemblyLoadContext.LoadFromStream to load an assembly from a Stream that is returned by FileSystem.OpenAppPackageFileAsync, I end up with a System.NotSupportedException on Android. I started with a ticket in Xamarin.Android: dotnet/android#7763. However, in the end it was determined that there is no way to reliably get the Length of an Asset Stream in Android.

I believe that in this function, it might be possible to avoid depending on the Stream.Length.

runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyLoadContext.cs

Line 397 in 6fecc25

static ReadOnlySpan<byte> ReadAllBytes(Stream stream)

Reproduction Steps

        using (var resStream = FileSystem.OpenAppPackageFileAsync("MyModule.dll").Result)
        {
            var loadCtx = new AssemblyLoadContext("Module");

            var assem = loadCtx.LoadFromStream(resStream);
        }

Expected behavior

LoadFromStream should not require knowing the Stream Length.

Actual behavior

LoadFromStream causes an exception due to requiring to get the Stream Length.

Regression?

No response

Known Workarounds

Copy the stream into a MemoryStream first.

Configuration

No response

Other information

No response

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

When using AssemblyLoadContext.LoadFromStream to load an assembly from a Stream that is returned by FileSystem.OpenAppPackageFileAsync, I end up with a System.NotSupportedException on Android. I started with a ticket in Xamarin.Android: dotnet/android#7763. However, in the end it was determined that there is no way to reliably get the Length of an Asset Stream in Android.

I believe that in this function, it might be possible to avoid depending on the Stream.Length.

runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyLoadContext.cs

Line 397 in 6fecc25

static ReadOnlySpan<byte> ReadAllBytes(Stream stream)

Reproduction Steps

        using (var resStream = FileSystem.OpenAppPackageFileAsync("MyModule.dll").Result)
        {
            var loadCtx = new AssemblyLoadContext("Module");

            var assem = loadCtx.LoadFromStream(resStream);
        }

Expected behavior

LoadFromStream should not require knowing the Stream Length.

Actual behavior

LoadFromStream causes an exception due to requiring to get the Stream Length.

Regression?

No response

Known Workarounds

Copy the stream into a MemoryStream first.

Configuration

No response

Other information

No response

Author:	MaxwellDAssistek
Assignees:	-
Labels:	bug, area-AssemblyLoader-coreclr, untriaged
Milestone:	-

[vitek-karas]

/fyi @elinor-fung

The problem is that we need to allocate a native memory buffer for the entire assembly. Knowing length means that we can avoid copying the memory twice - we only allocate the native buffer and read directly into it. Without knowing length we would have to read the stream first into some managed memory (I guess it could be a linked list of buffers to avoid reallocations) and then allocate and copy to native memory.

I think it would make sense to do so if we can detect that the stream doesn't have length. @grendello is there a way to tell if the stream from the mono/Android world supports length?

Note: this is probably not a regression, even before #72783 the code asked for length.

[grendello]

I think it would make sense to do so if we can detect that the stream doesn't have length. @grendello is there a way to tell if the stream from the mono/Android world supports length?

Unfortunately, no :(. We wrap Java streams in a managed Stream and so it always implements the Length property, but the underlying stream doesn't necessarily have one - that's why we throw the exception. About the only thing
we can do is to look at the docs and special-case those which support some kind of length information. For instance, we do it for FileInputStream right now, but in a rather roundabout way. FileInputStream doesn't have any method/field which tells us the size, but it owns a "channel" which, in turn, has a size() method. Other InputStream derivatives, e.g. ByteArrayInputStream stream have a field that gives the size, in this case count. So, as you can see, it's a bit convoluted... :)

I think we can add some special-cases to our code, but it will just work around some of the cases. However, perhaps you could catch the NotSupportedException (only on Android, there's no reason to hurt other platforms) and switch to the more expensive way of getting the stream size you described above, if it's caught? It's an expensive way, but it would work with all the unknown streams we don't special-case and which don't have the length info.

[vitek-karas]

How about CanSeek would that work as an indicator? The most common case currently I know if is loading assemblies from memory arrays (or resource streams), those will always support seeking, and thus we would use the more optimal code path. Special streams which don't support seek would go through the slower path.

[grendello]

@vitek-karas CanSeek may be useful, with caveats. InputStream has the seek() method which is the closest equivalent to (unidirectional, it seems) seeking. But our code in the wrapper has stricter requirements:

public override bool CanSeek { get { return (BaseFileChannel != null); } }

It will return only if the underlying stream is FileInputStream currently, because its channel has the Position() method which is the exact equivalent of Seek() in the managed stream. So even though querying CanSeek in to determine whether Length works is a bit of an abuse, I think it would work (at least for Android, where we can know this "convention" works and how)

[Maxwell175]

Pardon the suggestion, but could it be a valid solution to simply copy the stream to a MemoryStream outright and let it continue into the if that just gets a span from it? (only if the conditions match of course since copying the stream is probably more expensive)

[vitek-karas]

@Maxwell175 that's the idea, but we want to avoid doing it for the cases where it's not necessary. It's also a perfectly valid workaround on the caller side.

[stephentoub]

So even though querying CanSeek in to determine whether Length works is a bit of an abuse

It's not an abuse. CanSeek is documented to govern whether Length, SetLength, Position, and Seek are functional or may throw.

As was suggested, the method should use CanSeek. Basically:

if (stream.CanSeek)
{
   ... // existing path
   return ...;
}

var ms = new MemoryStream();
stream.CopyTo(ms);
ms.TryGetBuffer(out ArraySegment<byte> buffer);
return buffer;

or alternatively if the current path doesn't buy much:

var ms = new MemoryStream();
if (stream.CanSeek)
{
    ms.Capacity = stream.Length;
}
stream.CopyTo(ms);
ms.TryGetBuffer(out ArraySegment<byte> buffer);
return buffer;

[grendello]

@stephentoub CanSeek as a way to tell whether Length works is a bit of an abuse on Android, because we cannot guarantee that Seek, SetLength and Position will also work with Java streams, and that's contrary to the CanSeek docs.

[stephentoub]

CanSeek as a way to tell whether Length works is a bit of an abuse on Android, because we cannot guarantee that Seek, SetLength and Position will also work with Java streams, and that's contrary to the CanSeek docs.

I'm not understanding. CanSeek should only return true if all of Length, SetLength, Position, and Seek work... if any of them might fail, CanSeek should return false. Are you saying we ship a Stream that violates that?

[grendello]

CanSeek as a way to tell whether Length works is a bit of an abuse on Android, because we cannot guarantee that Seek, SetLength and Position will also work with Java streams, and that's contrary to the CanSeek docs.

I'm not understanding. CanSeek should only return true if all of Length, SetLength, Position, and Seek work... if any of them might fail, CanSeek should return false. Are you saying we ship a Stream that violates that?

That's why I said it was an abuse :) Java streams do not implement all of those operations. Some implement none of them, some only one etc. We ship a managed wrapper around those streams which does its best to present a "valid" managed stream to the user. @vitek-karas suggested CanSeek could be used to detect whether the stream supports Length and, indeed, it could - but we cannot guarantee the other operations won't throw NotSupportedException.