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 Crypto API stuck on long streams #1425

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

Conversation

antontroshin
Copy link

Description

The "Receive" was not called as the "Send" got stuck after about ~400kb of data was sent, it seems like a buffer was full, but since "Receive" wasn't called, the buffers weren't draining.
Once the "Send" task is not awaited, "Receive" is called and starts pulling data.
As long as "Send" is somehow being chained together with "Receive" (using Task.WhenAll, or .ContinueWith, etc), the "Receive" task is not starting.
The only way I was able to complete both was to disconnect them.
Ran 1000 iterations of EncryptAsync + DecryptAsync:
Using 6Mb file, total data processed ~12Gb (Encrypt 6Gb + Decrypt 6Gb)
No increase in memory was observed for daprd and tester app.
All went successful, the "Send" task was completed and cleaned.

Nevertheless, knowing that this is not a good practice when working with Tasks, would like to receive an opinion on this change or maybe an idea why this might happen with tasks.

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: #[issue number]

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

@antontroshin antontroshin requested review from a team as code owners December 12, 2024 20:09
…crypto-api-stuck-on-long-streams

# Conflicts:
#	src/Dapr.Client/DaprClientGrpc.cs
@WhitWaldo
Copy link
Contributor

Thank you for your report!

While the removal of Task.WhenAll would remove the blocking behavior you're experiencing, wouldn't you expect the operation to be blocking so that when the task returns, you can rely on an expectation that the full plaintext value was received and the full ciphertext value returned or vice versa (where in theory the encryption service should continue to operate as long as there's an inbound stream)?

By not awaiting the outbound side of the operation, exceptions wouldn't be propagated and any resources that would have been released because of the exception wouldn't release. Further, the use of Task.WhenAll means that when the provided cancellation token flags for cancellation, both operations can be cancelled. Here, if the receive operation completes (theoretically shouldn't happen) or is canceled first (could happen), the outbound side may not be properly canceled.

I think this solution needs more testing before we just merge it in. I'll get a larger file added for testing purposes and see if I can't reproduce your experience.

@WhitWaldo WhitWaldo added kind/bug Something isn't working area/client/crypto labels Dec 12, 2024
@WhitWaldo WhitWaldo modified the milestones: Future, Investigations Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/client/crypto kind/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants