Skip to content

Add Dapr.Cryptography + bugfix #1490

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

Open
wants to merge 7 commits into
base: release-1.16
Choose a base branch
from

Conversation

WhitWaldo
Copy link
Contributor

@WhitWaldo WhitWaldo commented Mar 17, 2025

Description

This PR implements three things:

  1. It splits out the cryptography functionality out to a separate Dapr.Cryptography package to be released with 1.16. All existing methods on Dapr.Client have an updated [Obsolete] attribute with a message telling developers that this method will be removed with the release of 1.17.
  2. An issue was reported as a PR which identified a race condition wherein either the read or write buffer could run out of content and the other stream would stall as they were filling and draining in tandem. This PR offers a fix to run either operation independently while still waiting for both to complete before completing the work - this PR applies to both the implementation in Dapr.Client and the one in Dapr.Cryptography.
  3. I identified a few small changes I could make for performance improvements and have applied these to both projects as well. The only difference is that Dapr.Cryptography wraps one of the methods in a try/catch block so as to better handle exceptions thrown by a CancellationToken, but this introduces another type that I didn't want to add to Dapr.Client.

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #1488 dapr/dapr#8244

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation

…mented fix for issue reported (need integration tests to validate).

Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
…hods to reflect future obsolescence and timetable for doing so

Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
@WhitWaldo WhitWaldo self-assigned this Mar 17, 2025
@WhitWaldo WhitWaldo requested review from a team as code owners March 17, 2025 19:16
Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
Signed-off-by: Whit Waldo <whit.waldo@innovian.net>

if (!shouldOmitDecryptionKeyName)
{
ArgumentException.ThrowIfNullOrEmpty(encryptionOptions.DecryptionKeyName,
Copy link
Contributor

Choose a reason for hiding this comment

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

Quick question, I think shouldOmitDecryptionKeyName will only be false in case string is not empty and has valid characters. In that case is this check required ?

get => streamingBlockSizeInBytes;
set
{
if (value <= 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also raise an illegal argument if the size is greater than 64 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The size could be configured to be larger than 64 as a configurable Dapr option and we wouldn't have any insight into it. If it's not configured and they pick too large a value, Dapr will throw an error accordingly.

Copy link
Contributor

@siri-varma siri-varma left a comment

Choose a reason for hiding this comment

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

Looks Good

@WhitWaldo
Copy link
Contributor Author

Looks Good

I appreciate it, but it's incomplete. Building out an example and documentation as well.

…fix the large-file stream concurrency issue (can reproduce locally)

Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
while ((bytesRead = await bufferedStream.ReadAsync(buffer.AsMemory(0, blockSize), cancellationToken)) !=
0)
{
await duplexStream.RequestStream.WriteAsync(
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for my understanding, could this be done in parallel as well ? Not requesting for any modification but was just trying to understand one vs another.

List<Task> tasks;
task.add(duplexStream.RequestStream.....)

await Task.whenAll(tasks)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This whole PR got a huge rewrite last night as it was still experiencing the reported issue of bombing out on large files, so review with a grain of salt.

While it could from a C# perspective be done in parallel with tasks, I don't think it would work with the Dapr runtime as it requires we submit the sequence number alongside the request payload and I believe it requires this to monotonically increase as it goes. It's not my recollection it will process chunks out of order. Also, I don't know that the response includes the sequence number, so re-assembly on the retrieval side could be problematic without some guarantee that the runtime is doing some reordering operation (and I don't know of any such guarantee).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants