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

CryptoStream.FlushFinalBlockAsync method missing #38076

Closed
rupertsciamenna89 opened this issue Jun 18, 2020 · 8 comments · Fixed by #39465
Closed

CryptoStream.FlushFinalBlockAsync method missing #38076

rupertsciamenna89 opened this issue Jun 18, 2020 · 8 comments · Fixed by #39465
Labels
api-approved API was approved in API review, it can be implemented area-System.Security
Milestone

Comments

@rupertsciamenna89
Copy link

rupertsciamenna89 commented Jun 18, 2020

Background and Motivation

I'm creating a web API for file encryption with an AES Key. I creates an helper functions that generate an instance of EncryptedFileResult (an extension of ActionResult class that I created) where I encrypt the file.

I override the two methods ExecuteResult and ExecuteResultAsync.

The body of ExecuteResultAsync is the next:

var fs = GetCryptoStream(ms, Cypher);
await fs.CopyToAsync(ms);
fs.FlushFinalBlock();
await fs.FlushAsync();

When the program runs, at the line fs.FlushFinalBlock();, it generates an exception:

'System.InvalidOperationException' in Microsoft.AspNetCore.Server.IIS.dll
Synchronous operations are disallowed. Call WriteAsync or set AllowSynchronousIO to true instead.

For the moment I set the AllowSynchronousIO to true, but it will be good to have an async overload for the FlushFinalBlock, to have the benefits of async methods.
I would like to implements it.

Proposed API

partial class CryptoStream
{
    public ValueTask FlushFinalBlockAsync(CancellationToken cancellationToken = default) => throw null;
}

Usage Examples

Previous code could be this:

var fs = GetCryptoStream(ms, Cypher);
await fs.CopyToAsync(ms);
await fs.FlushFinalBlockAsync();
await fs.FlushAsync();
@rupertsciamenna89 rupertsciamenna89 added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jun 18, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Threading.Tasks untriaged New issue has not been triaged by the area owner labels Jun 18, 2020
@ghost
Copy link

ghost commented Jun 18, 2020

Tagging subscribers to this area: @tarekgh
Notify danmosemsft if you want to be subscribed.

@vcsjones
Copy link
Member

cc @bartonjs

@ghost
Copy link

ghost commented Jun 18, 2020

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq
Notify danmosemsft if you want to be subscribed.

@vcsjones
Copy link
Member

vcsjones commented Jun 18, 2020

It looks like the FlushFinalBlock implementation is already async:

Though a public version is not directly exposed. However, DisposeAsync will perform the asynchronous flush final block.

Assuming you are done with the CryptoStream as well, you could use DisposeAsync to cause the final block to be flushed, asynchronously.

await FlushFinalBlockAsync(useAsync: true).ConfigureAwait(false);

This might be a good work around for now instead of setting AllowSynchronousIO.

@bartonjs
Copy link
Member

@stephentoub It doesn't look like it occurred to me to ask the question back then (dotnet/corefx#33422), but: Did you consider adding public (Value)Task FlushFinalBlockAsync(CancellationToken cancellationToken=default) when you added DisposeAsync()?

I'm guessing it was just a narrow scope of "override DisposeAsync in all the right places"

@stephentoub
Copy link
Member

Did you consider adding public (Value)Task FlushFinalBlockAsync(CancellationToken cancellationToken=default) when you added DisposeAsync()?

I don't think I considered it.

I'm guessing it was just a narrow scope of "override DisposeAsync in all the right places"

Yup.

@bartonjs
Copy link
Member

Seems like we can do

partial class CryptoStream
{
    public ValueTask FlushFinalBlockAsync(CancellationToken cancellationToken = default) =>
        FlushFinalBlockAsync(useAsync: true, cancellationToken);
}

And plumb the token into all of the relevant places in the existing private FlushFinalBlockAsync(bool)

@bartonjs bartonjs added api-ready-for-review and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation untriaged New issue has not been triaged by the area owner labels Jun 18, 2020
@bartonjs bartonjs added this to the 5.0.0 milestone Jun 18, 2020
@bartonjs bartonjs added the api-ready-for-review API is ready for review, it is NOT ready for implementation label Jul 6, 2020
@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jul 16, 2020
@terrajobst
Copy link
Member

terrajobst commented Jul 16, 2020

Video

Looks good as proposed.

namespace System.Security.Cryptography
{
    partial class CryptoStream
    {
        public ValueTask FlushFinalBlockAsync(CancellationToken cancellationToken = default);
    }
}

@dotnet dotnet locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Security
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants