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 not working as expected #6

Closed
savian-net opened this issue Jul 6, 2020 · 22 comments
Closed

Stream not working as expected #6

savian-net opened this issue Jul 6, 2020 · 22 comments
Labels
blazor server This is related to blazor server side good first issue Good for newcomers

Comments

@savian-net
Copy link

First of all, thanks for making this library. I still see a JS dependency in version 1.0.10 that is on nuget.

When I use this code:

        using var stream = new MemoryStream();
        wb.Save(stream, SaveOptions.XlsxDefault); //An Excel spreadsheet
        BlazorDownloadFileService.DownloadFile("SaviAnalysis-PROCs.xlsx", stream, "application/octet-stream");

It does not work and seems to treat the stream as empty. However, this code does work:

        using var stream = new MemoryStream();
        wb.Save(stream, SaveOptions.XlsxDefault);
        BlazorDownloadFileService.DownloadFile("SaviAnalysis-PROCs.xlsx", stream.ToArray(), "application/octet-stream");
@arivera12
Copy link
Owner

arivera12 commented Jul 6, 2020

Are you using Blazor Server or Blazor WASM?

And yes it uses JS under the hood cause its the only way to trigger browser download but

the JS code its embed on a class instead of being reference by the user in the html document.

Can you verify that the stream is not empty and the stream Position is 0 before sending the the DownloadFile method?

Based on your comment if stream.ToArray() works for you it seems that the stream position its at the end of the stream.

Try this:

using var stream = new MemoryStream();
wb.Save(stream, SaveOptions.XlsxDefault); //An Excel spreadsheet
stream.Position = 0; //Set the current position of the stream to the beginning
BlazorDownloadFileService.DownloadFile("SaviAnalysis-PROCs.xlsx", stream, "application/octet-stream");

@savian-net
Copy link
Author

savian-net commented Jul 6, 2020 via email

@arivera12 arivera12 added the blazor server This is related to blazor server side label Jul 6, 2020
@arivera12
Copy link
Owner

using var stream = new MemoryStream();
wb.Save(stream, SaveOptions.XlsxDefault); //An Excel spreadsheet
stream.Position = 0; //Set the current position of the stream to the beginning
BlazorDownloadFileService.DownloadFile("SaviAnalysis-PROCs.xlsx", stream, "application/octet-stream");

Try this please and let me know. If this throws an error please paste it over here it may be related to #5

@savian-net
Copy link
Author

It didn't. That one has bit me before so good call. Here is the code that I submitted:

        using var stream = new MemoryStream();
        wb.Save(stream, SaveOptions.XlsxDefault);
        stream.Position = 0;
        BlazorDownloadFileService.DownloadFile("SaviAnalysis-PROCs.xlsx", stream, "application/octet-stream");

It works fine with stream.ToArray()

@arivera12
Copy link
Owner

can you send me a sample so I can debug it?

As you can see in the internal implementation of stream overload method calls an extension method stream.ToByteArray() and then converts to base64 string internally cause JS runtime doesn't know how to deal with a stream or byte[].

See this issue: dotnet/aspnetcore#21877

@arivera12
Copy link
Owner

@AlanChurchill can you check if any error is being thrown in the browser console?

@arivera12 arivera12 added the good first issue Good for newcomers label Jul 6, 2020
@arivera12
Copy link
Owner

Check out if you are getting this in the console.
image

@savian-net
Copy link
Author

No error in Console. Yes, I saw that. Very limited time to debug since I have it working (and have my own unrelated bugs in my project). What you may want to consider doing is to not do the tobytearray extension. ToArray works and, from what little I read, it creates the byte array for you.

@arivera12
Copy link
Owner

@AlanChurchill the stream class doesn't provide a method .ToArray() but since you are using MemoryStream class, this class has the ToArray() method. I may have to consider 2 things use MemoryStream class instead stream class or reimplement the .ToByteArray() extension method.

@savian-net
Copy link
Author

savian-net commented Jul 6, 2020 via email

@arivera12
Copy link
Owner

Here is the internal implementation of ToByteArray(), not sure if using the CopyTo could be a little overhead

internal static byte[] ToByteArray(this Stream stream)
{
   var streamLength = (int)stream.Length;
   var data = new byte[streamLength + 1];
   stream.Read(data, 0, streamLength);
   stream.Close();
   stream.Dispose();
   return data;
}

@savian-net
Copy link
Author

Yeah, I looked at it last night. In general, I assume that the internal methods in Core are highly optimized. Minor cost even if a little less efficient. Worth testing or just try and see if it fixes the problem and move on to next issue in coding. Also, this is not something that will be called a lot so should not have to worry too much about it, IMO.

@arivera12
Copy link
Owner

@AlanChurchill any chance to send a sample which reproduces this error so I can debug it and fix it?

@arivera12
Copy link
Owner

How about using the CopyTo depending upon the type of stream: https://docs.microsoft.com/en-us/dotnet/api/system.io.stream.copyto?view=netcore-3.1

This seems good.
https://stackoverflow.com/a/6586039/3554970

@savian-net
Copy link
Author

I have replicated the basic code but I am not seeing same behavior with this. Hence, it will probably be on my shoulders to find out what is happening. You can see the code in this zip but need the GemBox.Spreadsheet dll (available from their website).

BlazorDownloadFile-WORKBOOK.zip

@arivera12
Copy link
Owner

I have replicated the basic code but I am not seeing same behavior with this. Hence, it will probably be on my shoulders to find out what is happening. You can see the code in this zip but need the GemBox.Spreadsheet dll (available from their website).

BlazorDownloadFile-WORKBOOK.zip

What you mean by not seeing the same behavior?

@arivera12
Copy link
Owner

I changed the implementation with the one I shared on the link, also added the overload for bufferSize and tested it on the demo project and seems ok. I am re implementing this library using buffer management to bypass Blazor Server side web socket max size I/O when working with large files.

Please follow this issue: #5

I will leave this issue open until you tell me if you had issue from your side or I re implement this and upload a new package version.

@savian-net
Copy link
Author

Thanks. What I saw was that the file would be written but was corrupt. When I looked at the binary, it seemed to be written with all null bytes. However, I didn't get too deep into it since I had other things I needed to work on. Once I got it to work with ToArray, off to next problem (you know the game).

So, to determine where an issue may lay, I may need to debug source.

@arivera12
Copy link
Owner

@AlanChurchill in the meantime as it is working with the ToArray method use it like that in the meantime. I will comment back here once I change implementation and re test everything again. Thanks for the report!

@arivera12
Copy link
Owner

arivera12 commented Jul 23, 2020

Install-Package BlazorDownloadFile -Version 1.0.12

@Smurf-IV
Copy link

Comments left as to why I do not like that version ....

@arivera12
Copy link
Owner

arivera12 commented Jul 24, 2020

@Smurf-IV the method you commented makes sense everything you said but take note that in this version the downloadfile method as overloads, which you can set the desired buffer. The extension method you commented about stream casting to memory stream it's the unbuffered method. For your use of case that extension method won't be used by you. The buffered methods has a total different implementation. Take a look of the current buffered implementantion of stream here

public async ValueTask DownloadFile(string fileName, Stream stream, int bufferSize = 32768, string contentType = "application/octet-stream")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blazor server This is related to blazor server side good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants