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

TransferUtilityUploadRequest.UploadProgressEvent event not firing #3308

Closed
Abhiram-Kasu opened this issue Apr 30, 2024 · 10 comments
Closed

TransferUtilityUploadRequest.UploadProgressEvent event not firing #3308

Abhiram-Kasu opened this issue Apr 30, 2024 · 10 comments
Assignees
Labels
bug This issue is a bug. p1 This is a high priority issue queued s3

Comments

@Abhiram-Kasu
Copy link

Describe the bug

Any delegates added to the UploadProgressEvent are not being activated. Files are still uploaded at the end, but callbacks are not being invoked. Tried downgrading sdk version to 3.7.0 and the callbacks DID WORK and were called.

Expected Behavior

Expected delegates to activate to give progress updates on the current file upload request

Current Behavior

Delegates are never called, as in the code inside the event handler is never ran regardless of the file size and the time taken

Reproduction Steps

Create a TransferUtility object

        var transferUtility = new TransferUtility(S3Client);

Create TransferUtilityUploadRequest with a file stream and attach handler to the UploadProgressEvent event

               var req = new TransferUtilityUploadRequest
                {
                    ContentType = "application/pdf",
                    InputStream = stream,
                    BucketName = "(Any bucket name)",

                };
                
                req.UploadProgressEvent +=  (sender, args) =>
                    Logger.LogInformation("Progress: {percent}", args.PercentDone);

await upload

                await transferUtility.UploadAsync(req);

Possible Solution

No response

Additional Information/Context

No response

AWS .NET SDK and/or Package version used

AWSSDK.S3 3.7.307.25

Targeted .NET Platform

.NET 8

Operating System and version

Mac OS Sonoma M3 Pro

@Abhiram-Kasu Abhiram-Kasu added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Apr 30, 2024
@Abhiram-Kasu Abhiram-Kasu changed the title TransferUtilityUploadRequest.UploadProgressEvent event not firing in latest sdk version TransferUtilityUploadRequest.UploadProgressEvent event not firing Apr 30, 2024
@bhoradc bhoradc self-assigned this Apr 30, 2024
@bhoradc bhoradc added needs-reproduction This issue needs reproduction. s3 and removed needs-triage This issue or PR still needs to be triaged. labels Apr 30, 2024
@ashishdhingra
Copy link
Contributor

ashishdhingra commented Apr 30, 2024

@Abhiram-Kasu Good morning. Could you please share the following:

  • Execution environment for your code.
  • What is the size of file that you are trying to upload?
  • You code uses InputStream in TransferUtilityUploadRequest object:
    • What is the source of this stream?
    • Is this stream seekable? We added support for uploading non-seekable stream few months back, in that case it might not be possible to capture the transfer progress since the stream is not seekable to get the file size.
    • Could you try uploading the file directly (instead of using the stream) by using FilePath property of TransferUtilityUploadRequest and see if callback delegate is invoked.

Thanks,
Ashish

@bhoradc bhoradc added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed needs-reproduction This issue needs reproduction. labels Apr 30, 2024
@Abhiram-Kasu
Copy link
Author

Hello @ashishdhingra,
The execution environment for my code is an ASP.NET .NET 8 Web App running in SSR and InteractiveServer. This code is called inside of a blazor page.

The size of the file that I am trying to upload is 12 mb, but I also have tried with bigger and smaller files

The source of the stream is from an IBrowserFile and from calling the OpenReadStream();

            await using var stream = file.OpenReadStream(MAX_FILE_SIZE);

As for trying to upload with a file, it does work when I copy the full file to the server, but it is not feasible for my blazor application as the file sizes can vary and grow.

Is there a way for me to achieve progress updates with a stream without copying the full file to the server?

@ashishdhingra
Copy link
Contributor

As for trying to upload with a file, it does work when I copy the full file to the server, but it is not feasible for my blazor application as the file sizes can vary and grow.

Is there a way for me to achieve progress updates with a stream without copying the full file to the server?

@Abhiram-Kasu We would need to investigate. So are you executing S3 upload using TransferUtility from the Blazor client side code? Could you check in .NET code, what is the value of CanSeek property of stream returned by file.OpenReadStream()?

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label May 1, 2024
@Abhiram-Kasu
Copy link
Author

The code is technically server-side code that runs over the web-scoket, if that helps anything. The CanSeek property did in fact return false. Here is any more information about the stream:
image

@ashishdhingra ashishdhingra added the needs-reproduction This issue needs reproduction. label May 1, 2024
@ashishdhingra
Copy link
Contributor

Reproducible using code below:

long MEG_SIZE = (int)Math.Pow(2, 20);
var fileName = GenerateName(@"SimpleUploadTest\SmallFile");
var path = Path.Combine(Path.GetTempPath(), fileName);
var fileSize = 20 * MEG_SIZE;
GenerateFile(path, fileSize);
//take the generated file and turn it into an unseekable stream

var stream = GenerateBrowserFileUnseekableStreamFromFile(path);
using (var tu = new Amazon.S3.Transfer.TransferUtility(client))
{
    TransferUtilityUploadRequest transferUtilityUploadRequest = new TransferUtilityUploadRequest()
    {
        BucketName = bucketName,
        Key = fileName,
        InputStream = stream
    };

    transferUtilityUploadRequest.UploadProgressEvent += (object? sender, UploadProgressArgs e) => Console.WriteLine($"Progress: {e.PercentDone}");
    transferUtilityUploadRequest.Metadata.Add("testmetadata", "testmetadatavalue");
    transferUtilityUploadRequest.Headers["Content-Disposition"] = "attachment; filename=\"" + fileName + "\"";

    tu.Upload(transferUtilityUploadRequest);
}

BrowserFileUnseekableStream GenerateBrowserFileUnseekableStreamFromFile(string filePath)
{
    try
    {
        BrowserFileUnseekableStream unseekableStream = new BrowserFileUnseekableStream(filePath);
        return unseekableStream;
    }
    catch (Exception ex)
    {
        Console.WriteLine($"An error occurred while generating the stream: {ex.Message}");
        throw;
    }
}

static string GenerateName(string name)
{
    return name + new Random().Next();
}

static void GenerateFile(string path, long size)
{
    string contents = GenerateTestContents(size);
    WriteFile(path, contents);
}

static void WriteFile(string path, string contents)
{
    string fullPath = Path.GetFullPath(path);
    new DirectoryInfo(Path.GetDirectoryName(fullPath)).Create();
    File.WriteAllText(fullPath, contents);
}

static string GenerateTestContents(long size)
{
    StringBuilder sb = new StringBuilder();
    for (long i = 0; i < size; i++)
    {
        char c = (char)('a' + (i % 26));
        sb.Append(c);
    }
    string contents = sb.ToString();
    return contents;
}

public class BrowserFileUnseekableStream : MemoryStream
{
    MemoryStream inner;
    public BrowserFileUnseekableStream(string path)
    {
        this.inner = new MemoryStream(File.ReadAllBytes(path));
    }

    public override bool CanSeek
    {
        get => false;
    }
    public override long Length
    {
        get
        {
            if (inner.Length >= -1)
            {
                return inner.Length;
            }
            throw new NotSupportedException();
        }
    }
}

Needs review with the team. Based on user provided screenshot in #3308 (comment), CanSeek for stream is false and Length has a value.

@ashishdhingra ashishdhingra added needs-review p2 This is a standard priority issue p1 This is a high priority issue queued and removed needs-reproduction This issue needs reproduction. needs-review p2 This is a standard priority issue labels May 2, 2024
@peterrsongg
Copy link
Contributor

peterrsongg commented May 22, 2024

@Abhiram-Kasu Hello, I've been trying to reproduce the scenario where you said it did work. You mentioned versions 3.7.0? Can you give me the exact version and maybe attach some logs for me to look at? I created a wrapper around the FileStream class where it wasn't seekable but did contain a content length. I tried to upload via the TransferUtility with version 3.7.0 and the upload wasn't even going through in those cases, so I'm curious how it was working in the previous versions.

When that didn't work I tried to use a Blazor app, using a BrowserFileStream and it threw this error at me:

System.InvalidOperationException: 'Base stream of PartialWrapperStream must be seekable'

Unseekable stream upload support was added in the last year or so, so the reason the delegates aren't being called now is because it's going through a different code path. We could add some progress handlers but first I want to make sure that it was working before and perhaps just route it to the previous code path where it was working.

@peterrsongg
Copy link
Contributor

peterrsongg commented May 22, 2024

The stream looks exactly the same, but for S3 version 3.7.0 it failes
unseekable_3 7 0_fail

EDIT: I see what's going on, you're uploading a file size below 16MB so it is triggering the simple upload. However with the recent change that we made to support unseekable streams it is defaulting to a multipart upload since the stream is not seekable. However, since your stream has a known length it's possible that we should be going down the old code path and also even possibly triggering a simple upload instead.. I don't require the sdk version anymore I have reproduced the scenario where it used to work.

@Abhiram-Kasu
Copy link
Author

Sorry for no response, glad to see you can reproduce. I did in fact just use version 3.7.0 though.

@peterrsongg
Copy link
Contributor

@Abhiram-Kasu No problem, the fix for this has been released in AWSSDK.S3 3.7.308.6. Going to close this out now, feel free to re-open if you find something wrong!

Copy link

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. p1 This is a high priority issue queued s3
Projects
None yet
Development

No branches or pull requests

4 participants