Make sure references are readable multiple times

[Alxandr]

When you reference a project containing precompiled Razor views from a project that uses a custom compiler (like F#), the result is often that the project get's compiled multiple times. Roslyn closes the resource streams when it's done with them, so therefore just using a single MemoryStream ends up with ObjectDisposedException on the second compilation. This commit fixes that.

[ghost]

Hi @Alxandr, I'm your friendly neighborhood Microsoft Open Technologies, Inc. Pull Request Bot (You can call me MSOTBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft Open Technologies, Inc. and real humans are currently evaluating your PR.

TTYL, MSOTBOT;

[rynowak]

We actually already have code sitting around for a stream implementation that wraps a stream and prevents disposal. That would avoid creating the extra copy.

https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNet.Mvc.Core/Internal/NonDisposableStream.cs

[Alxandr]

MemoryStream doesn't copy the buffer though: http://referencesource.microsoft.com/#mscorlib/system/io/memorystream.cs,92.

So not really sure which would be smaller.

[rynowak]

It's the ToArray() to create the buffer that makes a copy.

This might be irrelevant anyway, would this be accessed concurrently or re-entrantly? If that's the case what you're doing is necessary.

[Alxandr]

I don't know. Depends if dnx locks emits or not. Though it's the Roslyn immutable api that consumes it, so it might be expected to be parallizable

[dnfclas]

Hi @Alxandr, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. Real humans will now evaluate your PR.

TTYL, DNFBOT;

[dnfclas]

@Alxandr, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

[pranavkm]

I don't know. Depends if dnx locks emits or not.

cc @davidfowl . AFAIK it does.

[Alxandr]

Could I get some more feedback here? Do you want me to change it to use NonDisposableStream?

[pranavkm]

Could we perhaps start by re-seeking the stream:
() => new MemoryStream(assemblyBytes) ▶️ () => { assemblyStream.Position = 0; return assemblyStream; }

We can change it to create a new stream if we do see issues during compilation.

[Alxandr]

@pranavkm No, because it's disposed by roslyn (ie, roslyn uses it in a using block). The problem isn't that we're at the end of the stream, but that it's closed.

[pranavkm]

Ok, that makes sense. One of the things @DamianEdwards wants to discuss as part of the precompilation discussion today is to determine if we should conditionally precompile views (as part of release \ prod builds, deployment) rather than attempting to support it at dev time. If we decide this is the way forward, the extra allocation here would be less of a concern.

Let's wait until #2463 (comment) is completed to figure out how to proceed with this work item.

[Alxandr]

Is there any good reason this is still being held off?

[Eilon]

@Alxandr no particular reason, sorry!

@pranavkm you're Mr. Compilation, can you review?

[pranavkm]

Nope. I just haven't gotten around to getting this in. That said, I think switching to NonDisposableStream should be a safe change here. Since the original streams are MemoryStream instances, simply wrapping them in a NonDisposableStream before passing it into Roslyn should suffice.

[Alxandr]

Do the non disposable stream reset position on every use?

On Tue, Aug 11, 2015, 18:50 Pranav K [email protected] wrote:

Nope. I just haven't gotten around to getting this in. That said, I think
switching to NonDisposableStream should be a safe change here. Since the
original streams are MemoryStream instances, simply wrapping them in a
NonDisposableStream before passing it into Roslyn should suffice.

—
Reply to this email directly or view it on GitHub
#2461 (comment).

Alxandr

[pranavkm]

Nope. All it does is no-op the Dispose. I guess it would look like:

() => { assemblyStream.Position = 0; return new NonDisposableStream(assemblyStream); }

[Alxandr]

This would completely break in parallel though... But that might not be an
issue?

On Tue, Aug 11, 2015, 19:51 Pranav K [email protected] wrote:

Nope. All it does is no-op the Dispose. I guess it would look like:

() => { assemblyStream.Position = 0; return new
NonDisposableStream(assemblyStream); }

—
Reply to this email directly or view it on GitHub
#2461 (comment).

Alxandr

[pranavkm]

@Alxandr, I guess we would have similar issues if the stream was being read in parallel now.

[Alxandr]

@pranavkm Yeah, it can't be read in parallel, but you can request multiple in parallel, and read them by themselves. Inside Roslyn these streams are used as using (Stream stream = _streamProvider()). If you run that in parallel using the NonDisposableStream, you end up with them interfering with eachother (they don't have each their own position counter). But as said, this might not be an issue.

[pranavkm]

Let's start with assuming they don't get requested in parallel. We could always fix this later on.

[Alxandr]

Yeah, that's probably reasonable. Though, given that this is literally just
2 lines of code that should be changed from the mvc dev branch, would you
mind fixing it, and just rejecting this pr? I unfortunately just sent my
laptop away for support, and have no idea when it's going to be back.

On Wed, Aug 12, 2015, 23:38 Pranav K [email protected] wrote:

Let's start with assuming they don't get requested in parallel. We could
always fix this later on.

—
Reply to this email directly or view it on GitHub
#2461 (comment).

Alxandr

[pranavkm]

👍

[pranavkm]

Merged in 39ab9ba