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: prevent multiple downloads of same file #2477

Merged
merged 8 commits into from Jun 24, 2019

Conversation

3 participants
@bartlomieju
Copy link
Contributor

commented Jun 8, 2019

This PR is proof-of-concept to prevent multiple downloads of same file as described in #2442.

For example given in #2442, here's downloads before this patch:

deno_dev run --reload --allow-read test.ts
[192/192] Compiling https://raw.githubusercontent.com/denoland/deno_std/v0.7.0/http/server.ts

And after patch:

deno_dev run --reload --allow-read test.ts
[39/39] Compiling https://raw.githubusercontent.com/denoland/deno_std/v0.7.0/media_types/mod.ts

It builds on idea of State.compiled - once file is downloaded it's placed in HashSet to prevent another download of same file. Code is hacky to prevent necessity of huge changes in DenoDir - I don't intend this to be merged but rather be another POV for refactor of DenoDir.

CC @kitsonk

Fixes #2442

@kitsonk

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2019

It makes sense to me, but I don't know if I am really the best authority. My Rust knowledge is just enough to be dangerous.

@bartlomieju bartlomieju referenced this pull request Jun 9, 2019

Open

Rewrite deno_dir.rs #2057

@hayd hayd referenced this pull request Jun 16, 2019

Open

deno fmt is slow #2490

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented Jun 16, 2019

@ry could you take a look at this PR?

Tests are failing but I believe they are not replicating "real life" scenario - I don't think it's possible to change headers for certain file during same run - then we'd need to change setup of tests a little bit.

Also I had other idea: update fetch_remote_source_async so it caches whole result of download (including code, content type and redirects).

Show resolved Hide resolved cli/deno_dir.rs Outdated

@bartlomieju bartlomieju force-pushed the bartlomieju:test-prevent_multiple_downloads branch from 4889a5c to d14adf2 Jun 23, 2019

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2019

@ry I think I managed to find solution using initial approach that involves marking which URLs had already been downloaded. It's much simpler compared to saving whole response.

I changed two tests to behave like a real world scenario and added a test for multiple downloads (verifying that headers file modified timestamp is unchanged). Let me know what you think.

But as you said earlier this is definitely to be rewritten from ground up.

@bartlomieju bartlomieju changed the title prevent multiple downloads of same file [do not merge] fix: prevent multiple downloads of same file Jun 23, 2019

Show resolved Hide resolved cli/deno_dir.rs Outdated
Show resolved Hide resolved cli/deno_dir.rs
Show resolved Hide resolved cli/deno_dir.rs Outdated
Show resolved Hide resolved cli/deno_dir.rs
let headers_file_metadata_2 = headers_file_2.metadata().unwrap();
let headers_file_modified_2 = headers_file_metadata_2.modified().unwrap();

assert_eq!(headers_file_modified, headers_file_modified_2);

This comment has been minimized.

Copy link
@ry

ry Jun 24, 2019

Collaborator

Very nice! I've been trying to come up with a test for this failure for a long time. I have verified this test does indeed fail when applied to master.

Thank you for solving this bug!

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Jun 24, 2019

Author Contributor

Very nice! I've been trying to come up with a test for this failure for a long time. I have verified this test does indeed fail when applied to master.

Thank you for solving this bug!

Great, thanks! 🎉

@ry
Copy link
Collaborator

left a comment

LGTM modulo a few nits...

bartlomieju added some commits Jun 24, 2019

@ry

ry approved these changes Jun 24, 2019

@ry ry merged commit 3c81cca into denoland:master Jun 24, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@bartlomieju bartlomieju deleted the bartlomieju:test-prevent_multiple_downloads branch Jun 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.