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

Stream blob back to client rather than read into memory. #156

Merged
merged 4 commits into from
Jan 28, 2020
Merged

Stream blob back to client rather than read into memory. #156

merged 4 commits into from
Jan 28, 2020

Conversation

darrenferguson
Copy link

Was getting out of memory exceptions on large files - shouldn't need to read the whole blob into memory before returning.

@stevetemple stevetemple self-requested a review January 16, 2020 13:44
Copy link
Collaborator

@stevetemple stevetemple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very sensible to me, definitely a worthwhile addition. Thanks @darrenferguson

Jeavon added a commit that referenced this pull request Jan 28, 2020
@Jeavon Jeavon changed the base branch from master to develop January 28, 2020 10:26
@Jeavon Jeavon merged commit 0cd5632 into umbraco-community:develop Jan 28, 2020
Jeavon added a commit that referenced this pull request Feb 19, 2020
… into memory. - thanks Darren"

This reverts commit 9aa46c6.
@darrenferguson
Copy link
Author

@Jeavon marc said you may have an issue with this and large files? Strange because we are running it in prod with some quite big files... what have you encountered?

@bruno-casari-shout
Copy link

@darrenferguson @stevetemple
Has this issue been addressed? It looks like the change has been reverted on version 1.1.1?

We are getting OutOfMemory exceptions as well when large files (MP4 videos) are being retrieved from Azure Blob Storage (on version 1.0.2). I am assuming it may be due to the issue explained on this thread - all MP4 file being loaded to MemoryStream before writing response back to the client?

@Jeavon
Copy link
Collaborator

Jeavon commented Jun 10, 2020

@Jeavon marc said you may have an issue with this and large files? Strange because we are running it in prod with some quite big files... what have you encountered?

Yes, we had to revert this change as OpenRead() will only read the first 4mb of a blob - https://stackoverflow.com/questions/6911728/cloudblob-openread-is-not-reading-all-data

You can see interesting things like this:
image (3)

@darrenferguson
Copy link
Author

Yes, we had to revert this change as OpenRead() will only read the first 4mb of a blob - https://stackoverflow.com/questions/6911728/cloudblob-openread-is-not-reading-all-data

4MB is the default block size of Microsoft.WindowsAzure.Storage.Blob.CloudBlob - there is a property you can set called - StreamMinimumReadSizeInBytes.

So I guess the issue lies in how Umbraco manipulates the stream.

I ended up working around the issue by rewriting requests to large media files to something like this: https://gist.github.com/darrenferguson/efda4d68d535c9e4a67963bb847c042e

As you can see it still returns a Stream using OpenRead() but the FileStreamResult class handles the reading of the stream in chunks and returning it to the client correctly.

I'll have a dig into Umbraco and see if I can work out what it is doing. I guess it makes sense that it worked for me in some cases, as different parts of Umbraco may manipulate the stream returned from the file system provider in different ways.

@Jeavon
Copy link
Collaborator

Jeavon commented Jul 14, 2020

@darrenferguson ah I see, I think then that the VPP code would need to be updated to utilise the FileStreamResult

@JimBobSquarePants
Copy link
Contributor

I wouldn't think you should use a FileStreamResult as part of the VPP, rather I would look at how media itself is served to the response in Umbraco. FileStreamResult in itself does nothing of interest, it's the executor that does the stream copying by pooling an interim buffer and reading/writing to and from that buffer.

This is how the copy occurs in ASP.NET Core, I'm sure it'll be very similar in classic.
https://source.dot.net/#Microsoft.AspNetCore.Http.Extensions/StreamCopyOperationInternal.cs,43a14722ac80264f

So blockBlob.OpenRead() without the copy is correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants