Embedio: an issue in FileModuleBase for handling Partial Content which lead to not resuming properly

[madnik7]

Describe the bug
Current version does not resume file downloading by some client and the clients restart downloading the entire file

Reason
I have checked FileModuleBase.WriteFileAsync code and just found the problem,
Some client like "Free Download Manager" request partial content on resuming like this "bytes=28147712-" so the client just mentions the start index and the rest of the file but the current implementation just add partial headers when the end of file is different as follow:

if (upperByteIndex != fileSize) //** BUG HERE
            {
                response.StatusCode = 206;
                response.ContentLength64 = upperByteIndex - lowerByteIndex + 1;

                response.AddHeader(Headers.ContentRanges,
                    $"bytes {lowerByteIndex}-{upperByteIndex}/{fileSize}");
           }

Code Fix
the code should check the lowerByteIndex too

if (lowerByteIndex != 0 || upperByteIndex != fileSize) //**Change: lowerByteIndex != 0 ||
{
   .....
}

Expected
I hope to have it fixed in the next release so I revert my custom code

[geoperez]

Good catch, let you know when the nuget is ready.

[geoperez]

You can check new nuget v2.2.8!

[madnik7]

Thank you! Now there is another problem which I haven't found it yet. I have mp4 which is seekable as I tested in IIS with no problem. but even with latest embedio version (2.2.8) the latest google chrome can not seek through the movie. it looks it use more advanced approach of partial content or maybe use some extra http header!

I am working on it, any comment is welcome.

[madnik7]

OK, I've found the problem. FileModuleBase.CalculateRange has a critical issue. it set upperByteIndex to fileSize whilst it should be fileSize-1! also I think it doesn't handle some other case properly too.

by the way, I have refactored it to the following using .net standard class:

private static bool CalculateRange(string partialHeader, long fileSize, out long lowerByteIndex, out long upperByteIndex)
        {
            try
            {
                var range = RangeHeaderValue.Parse(partialHeader).Ranges.First();
                lowerByteIndex = range.From ?? 0;
                upperByteIndex = range.To ?? fileSize - 1;
                return true;
            }
            catch
            {
                lowerByteIndex = 0;
                upperByteIndex = fileSize - 1;
                return false;
            }
        }

would you fix this error and publish it?

[geoperez]

I didn't know about that class, much better approach! I updated the code, and I'll later publish the nuget.

Thanks!

[madnik7]

I am not sure about your programming style, but you can also simplify FileModuleBase.WriteToOutputStream to

public static Task WriteToOutputStream(this IHttpResponse response, Stream buffer, long lowerByteIndex=0, CancellationToken ct = default)
        {
            buffer.Position = lowerByteIndex;
            var ret = buffer.CopyToAsync(response.OutputStream, Modules.FileModuleBase.ChunkSize, ct);
            buffer.Dispose();
           return ret;
        }

also, not sure why you dispose the buffer here, because usually the method who create the object should dispose it.

[geoperez]

I like the idea to use CopyToAsync, regarding the dispose is because we faced some issues with the MemoryStream.

[madnik7]

the dispose is because we faced some issues with the MemoryStream.

Indeed, the MemoryStream should be disposed by the way, but perhaps at the end of request by the object which has created the MemoryStream at first. I can imagine when an exception occur or if in some situation where WriteToOutputStream is not called, the created MemoryStream might not be disposed and may lead to unexpected behavior or resource leak .

Also, as a rule of thumb, callers usually don't expect the passing object be disposed by the method.

[geoperez]

Let me think how to order the dispose of the MemoryStream.

[geoperez]

I did some changes, and the unit tests are passing... So I guess it's fine.

[geoperez]

What do you think? Should we publish?

[madnik7]

So, i checked the commits! I couldn't find where the stream is disposed.

Personally, I don't like the .ConfigureAwait(false) too. some people think it is a good practice but I don't.

[geoperez]

I added a using statement when the MemoryStream is created. What I know it's that you should always use the ConfigureAwait(false) when the code is a third party code: https://blog.stephencleary.com/2012/07/dont-block-on-async-code.html

[madnik7]

I added a using statement when the MemoryStream is created

Nice, I didn't noted.

I know it's that you should always use the ConfigureAwait(false) when the code is a third party code: https://blog.stephencleary.com/2012/07/dont-block-on-async-code.html

Ya, and some other like:
https://medium.com/bynder-tech/c-why-you-should-use-configureawait-false-in-your-library-code-d7837dce3d7f

some other counter approach too:
https://blogs.msdn.microsoft.com/benwilli/2017/02/09/an-alternative-to-configureawaitfalse-everywhere/

But, I still don't like it, it is the caller responsibility to manage it. it ruins the chain of async method! If it was a good idea, it would be done by .net standard library too. Why CopyToAsync doesn't manage it internally!?
I don't like to write bunch of code just because the caller don't know how to manage an async methods. By the way, if the caller doesn't know how to manage async methods then he/she definitely does same mistake by calling thousands other .net library async methods.

[geoperez]

Yeah, I'm not a big fan to end all the async sentence with ConfigureAwait, but we try to keep it simple for the final user. It's hard for beginners to understand how async/await works properly.

[madnik7]

Ya, but in our case it rarely happens, because it is a webserver and there is no UI thread is most cases. again if users don't know how to handle async, then they can't work with other .net libraries too. I think we don't solve the problem by ConfigureAwait.

[geoperez]

I disagree about the UI because I know that some developers are using the web server inside Xamarin Forms.

[madnik7]

Actually, I am using Xamarin too :), but if I run a webserver, it means I plan to connect it to via HTTP and don't call their method directly in UI.

[geoperez]

I'm going to release the nuget unless you have another comment.

I'll discuss later with my team if we need to change the ConfigureAwait approach.

[madnik7]

I don't have any comment at the moment.
Thanks

[geoperez]

One question, did you test another web server before EmbedIO?

[madnik7]

Ya, IIS and Kestrel, but they can't run on .net standard both on android and windows. It is a valuable project that help me to run personal webserver on both windows and mobile devices. :)

[geoperez]

Cool! The new nuget v2.2.9 is out!

[geoperez]

@madnik7 can I close this issue?

[madnik7]

Sure, why not!

On Tue, Feb 26, 2019, 7:36 AM Geovanni Perez [e-mail]
wrote:

@madnik7 https://github.com/madnik7 can I close this issue?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/unosquare/embedio/issues/247#issuecomment-467486731,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ADCsK7Zpl8TMq9DDcqHh-yfsHCXGaD7Aks5vRVSGgaJpZM4bEDje
.