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

INT-790: Remove extra await #257

Merged
merged 5 commits into from
Dec 11, 2023
Merged

Conversation

ntrevino-virtru
Copy link
Contributor

@ntrevino-virtru ntrevino-virtru commented Dec 4, 2023

This is to fix large file downloads from what I understand from @ivanovSPvirtru 😅

@@ -1061,7 +1061,8 @@ export async function readStream(cfg: DecryptConfiguration) {

const cipher = new AesGcmCipher(cfg.cryptoService);

await updateChunkQueue(
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. This means if there is a network error in this function, it won't propagate anymore. Instead of throwing, you will have to explicitly call a _reject handler on the chunk.

  • In this method: add a corresponding _reject parameter, in addition to the _resolve handler below. This then propagate an error in chunk processing to the stream assembly code here by throwing it out on the await on line 1085
      if (!chunk.decryptedChunk) {
        await new Promise((resolve, reject) => {
          chunk._resolve = resolve;
          chunk._reject = reject;
        });
      }
  • Above, in updateChunkQueue, instead of throwing the error, send it to the offending slice. It is kinda hard, since you won't know which slice the error happened on in the current double-looped code. So you'll have to refactor it a bit so the handler happens within the innermost loop, and add another handler for the zip format check which just attaches it to the first item in the stride.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it should be hooked up properly, though not sure exactly which zip format check you meant.

Copy link
Contributor

@ivanovSPvirtru ivanovSPvirtru Dec 7, 2023

Choose a reason for hiding this comment

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

Little context

  1. Without await its starts in parallel with file writing stream (decrypt and download) and population of chunks map with file chunks
  2. If they run in parallel stream eats basically those chunks from map as soon as they populate so memory never overlflow. Its dowloading, populating map writing stream instantly
  3. If we wait that function, its just polulate map and when its populated with file chunks then it gets writed to hard drive
  4. Its fine if file less then 5 gb but if more than its reaches limit of size for browser keeping stuff in memory
    Why it didnt failed before - because it was instantly written to harddrive and removed from browser memory

Copy link

github-actions bot commented Dec 5, 2023

If these changes look good, signoff on them with:

git pull && git commit --amend --signoff && git push --force-with-lease origin

If they aren't any good, please remove them with:

git pull && git reset --hard HEAD~1 && git push --force-with-lease origin

@ntrevino-virtru ntrevino-virtru force-pushed the feature/int-790/remove-extra-await branch from d10c301 to 2818e0b Compare December 6, 2023 18:42
const encryptedChunk = new Uint8Array(
buffer.slice(offset, offset + (encryptedSegmentSize as number))
);
return Promise.all(
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunetly it wont change anything. When you do Promise.All on decrypt its not paralelise decryption. I tried it donesnt give any peformnace boost.

https://github.com/opentdf/client-web/pull/256/files#diff-5c6d9ea24cc7a3470e79efacc4f7aed59b657798a7cba23499d0cfd6476b9935R970

here work that im doing for that uses workers. But without limiting promise.all of lot of chunks can freeze ui so i use pLimit.

Could you remove Promise.all for now since changes is on the way? and leave just _reject part?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@ntrevino-virtru ntrevino-virtru force-pushed the feature/int-790/remove-extra-await branch from 2818e0b to 9b22c67 Compare December 7, 2023 15:27
Copy link

github-actions bot commented Dec 7, 2023

If these changes look good, signoff on them with:

git pull && git commit --amend --signoff && git push --force-with-lease origin

If they aren't any good, please remove them with:

git pull && git reset --hard HEAD~1 && git push --force-with-lease origin

@ntrevino-virtru ntrevino-virtru force-pushed the feature/int-790/remove-extra-await branch from 907842e to a5a17d4 Compare December 7, 2023 15:33
@@ -946,7 +947,7 @@ async function updateChunkQueue(
bufferSize
);
if (buffer) {
sliceAndDecrypt({
await sliceAndDecrypt({
Copy link
Contributor

Choose a reason for hiding this comment

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

We dont need await here. Dowload (getPayloadSegment) doesnt need to wait for dowloaded chunk to decrypt to start to dowload next chunk. Its just slowedown file download.
image

stream waits for new chunk in chun map, slice and decrypt provide it. Dowload doesnt have to stop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the explanation! Updated!

ivanovSPvirtru
ivanovSPvirtru previously approved these changes Dec 7, 2023
@ntrevino-virtru
Copy link
Contributor Author

There was still an issue with decrypts, but I was able to test in SS and it seems fine to me so far.

Copy link

sonarqubecloud bot commented Dec 8, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

Copy link
Contributor

@ivanovSPvirtru ivanovSPvirtru left a comment

Choose a reason for hiding this comment

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

10gb file successfully decrypted. No memory leak

Copy link
Member

@dmihalcik-virtru dmihalcik-virtru left a comment

Choose a reason for hiding this comment

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

You could rewrite it to not use .then, but the logic seems correct as is if you'd rather not

@ntrevino-virtru ntrevino-virtru merged commit 2decf73 into main Dec 11, 2023
@ntrevino-virtru ntrevino-virtru deleted the feature/int-790/remove-extra-await branch December 11, 2023 15:47
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.

3 participants