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

Fix support of webassembly #911

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix support of webassembly #911

wants to merge 4 commits into from

Conversation

Chang228
Copy link

@Chang228 Chang228 commented Nov 8, 2023

Webassembly has no support HMAC calculation and HttpResponseMessage sync stream,
use code to implement HMAC SHA256 calculation

@Chang228 Chang228 closed this Nov 11, 2023
@Chang228 Chang228 reopened this Nov 11, 2023
@Chang228 Chang228 closed this Nov 11, 2023
Copy link
Author

@Chang228 Chang228 left a comment

Choose a reason for hiding this comment

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

Review

@Chang228 Chang228 closed this Nov 11, 2023
@ebozduman
Copy link
Collaborator

@Chang228,
Sorry for the late review.
I'd like to reopen the PR and send you the belated review comments.

@ebozduman ebozduman reopened this Nov 20, 2023
Copy link
Collaborator

@ebozduman ebozduman left a comment

Choose a reason for hiding this comment

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

@Chang228,
Here are my review changes as a git diff output:

$ git diff Minio/DataModel/Result/ResponseResult.cs
diff --git a/Minio/DataModel/Result/ResponseResult.cs b/Minio/DataModel/Result/ResponseResult.cs
index f4ad4c3..5b4c062 100644
--- a/Minio/DataModel/Result/ResponseResult.cs
+++ b/Minio/DataModel/Result/ResponseResult.cs
@@ -31,7 +31,7 @@ public ResponseResult(HttpRequestMessage request, HttpResponseMessage response,
     {
         Request = request;
         Response = response;
-        ContentStream = stream;
+        ContentStream = stream ?? Task.Run(async () => await (Response?.Content.ReadAsStreamAsync()).ConfigureAwait(false)).Result;
     }
 
     public ResponseResult(HttpRequestMessage request, Exception exception)
@@ -56,7 +56,7 @@ public HttpStatusCode StatusCode
         }
     }
 
-    public Stream ContentStream { get; }//if (Response is null) return null;//return stream ??= Response.Content.ReadAsStream();
+    public Stream ContentStream { get; }
 
     public ReadOnlyMemory<byte> ContentBytes
     {

Comment on lines +99 to +108
var statusCode = response.StatusCode;
var memoryStream = new MemoryStream();
if (statusCode == HttpStatusCode.OK)
{
var stream = await response.Content.ReadAsStreamAsync().ConfigureAwait(false);
await stream.CopyToAsync(memoryStream).ConfigureAwait(false);
memoryStream.Position = 0;
}

responseResult = new ResponseResult(request, response, memoryStream);
Copy link
Collaborator

@ebozduman ebozduman Nov 20, 2023

Choose a reason for hiding this comment

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

Because of the previous review comments, this part needs to be cleaned up as responseResult has the ContentStream already set up.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You should also run dotnet regitlint for formatting problems.

@@ -202,14 +202,69 @@ private ReadOnlySpan<byte> GenerateSigningKey(string region, DateTime signingDat
/// <param name="content">Bytes to be hmac computed</param>
/// <returns>Computed hmac of input content</returns>
private ReadOnlySpan<byte> SignHmac(ReadOnlySpan<byte> key, ReadOnlySpan<byte> content)
{
return ComputeHmac256(key.ToArray(), content.ToArray());
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this code is the cause of the Functional test failure caught during the build tests.

You need to debug the code and resolve the issue.
When I reverted this change and run the Functional tests with the code suggested in the previous review comments, Functional tests ran successfully.

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.

2 participants