fix: limit the max amount of concurrent network requests #116

zkochan · 2023-09-16T00:46:09Z

Without limiting the amount of concurrent network requests, installation fails with too many open files error

github-actions · 2023-09-16T00:52:17Z

Benchmark Results

Linux

group                          main                                   pr
-----                          ----                                   --
tarball/download_dependency    1.00      7.9±2.88ms   549.4 KB/sec    1.00      7.9±2.81ms   551.8 KB/sec

KSXGitHub · 2023-09-16T18:02:27Z

crates/cli/src/commands/install.rs

-                .unwrap();
-                self.install_dependencies(&dependency).await;
+            .map(|(name, version)| {
+                let semaphore_clone = semaphore.clone();


I recommend Arc::clone(&semaphore) to avoid confusing it with regular expensive .clone().

KSXGitHub · 2023-09-16T18:09:27Z

Is this related to the error on macOS?

KSXGitHub · 2023-09-16T18:21:21Z

crates/cli/src/commands/install.rs

@@ -55,17 +57,19 @@ impl PackageManager {
        package
            .dependencies(self.config.auto_install_peers)
            .map(|(name, version)| async {
+                let semaphore_clone = semaphore.clone();


semaphore is not an Arc here. Cloning it would create a new Semaphore. Is this intentional?

zkochan · 2023-09-16T20:25:55Z

Is this related to the error on macOS?

This is one of the errors on macOS. The other one is failed to lookup address information: nodename nor servname provided, or not known. I don't have a fix for that one yet.

Also, this PR is not ready yet. Looks like this change is causing a deadlock.

anonrig

Can you add a test so that we don't regress in the future?

anonrig · 2023-09-16T23:16:50Z

crates/cli/src/commands/add.rs

@@ -59,13 +60,15 @@ impl PackageManager {
    /// 5. Symlink all dependencies to node_modules/.pacquet/pkg@version/node_modules
    /// 6. Update package.json
    pub async fn add(&mut self, args: &AddCommandArgs) -> Result<(), PackageManagerError> {
+        let semaphore = Arc::new(Semaphore::new(16));


I think this should be configurable somehow

KSXGitHub · 2023-09-17T04:28:06Z

Maybe related? https://superuser.com/questions/433746/is-there-a-fix-for-the-too-many-open-files-in-system-error-on-os-x-10-7-1

KSXGitHub · 2023-09-17T06:17:41Z

Looks like this change is causing a deadlock.

I don't get deadlock on my machine. What is your package.json?

zkochan · 2023-09-17T09:27:53Z

I am using this package.json

KSXGitHub · 2023-09-17T12:28:58Z

The error is not unique to macOS, it seems.

zkochan · 2023-09-17T12:34:37Z

This is a different one. I also get this one. pnpm solves this by using graceful-fs, which retries the file system operations on these type of errors.

KSXGitHub · 2023-09-17T12:35:10Z

crates/registry/src/package.rs

    ) -> Result<Self, RegistryError> {
        let url = || format!("{registry}{name}"); // TODO: use reqwest URL directly
        let network_error = |error| NetworkError { error, url: url() };
+        let _permit = semaphore.acquire().await;


Is there a reason you gave this value a name? If you want to postpone dropping it, I recommend explicitly calling drop (i.e. drop(permit)) at the end of the scope. If you only want to .await the semaphore, please remove let _permit =.

won't it be dropped immediately if I don't assign it to a value? As far as I understand, the permit is dropped, when it goes out of scope.

KSXGitHub · 2023-09-17T12:38:55Z

Looks like this change is causing a deadlock.

I am using this package.json

After updating the package.json and switching branch to network-concurrency (this branch), what I get is not a deadlock, but instead (it seems) an infinite loop that logs endlessly.

You may try it with TRACE='pacquet::install,pacquet::download' to see for yourself.

KSXGitHub · 2023-09-19T17:17:12Z

If I set the semaphore number to 1000, this error appears:

Error:   × Main thread panicked.
  ├─▶ at crates/cli/src/package_import.rs:38:26
  ╰─▶ expected no write errors: CreateLink { from: "/home/khai/Temp/test-pacquet/pacquet-home/store/7c/a8bebf69ba6047dd7be009132ae6a30992062657634c1573fa04f35cb9de7b31d4316606d61f748300eddc0cc91220ccaa8562814d430d218a6bd0a13471e2", to:
      "/home/khai/Temp/test-pacquet/node_modules/.pacquet/[email protected]/node_modules/dom-helpers/util/inDOM.js", error: Os { code: 24, kind: Uncategorized, message: "Too many open files" } }

But if I don't, the install process never stops.

KSXGitHub · 2023-09-19T17:18:57Z

I also created a more "polished" version here: df491ea

Same result.

zkochan · 2023-09-19T21:34:14Z

I think the issue might be that pacquet doesn't support circular dependencies yet.

KSXGitHub · 2023-09-20T00:34:04Z

I think the issue might be that pacquet doesn't support circular dependencies yet.

It would helps a lot if you can reduce your package.json to only circular dependencies (for testing and implementing purposes).

zkochan · 2023-09-20T09:32:17Z

As I have mentioned in #124, there are tests in pnpm related to circular dependencies. And there are small mock packages for testing: https://github.com/pnpm/registry-mock/tree/main/packages/circular-deps-1-of-2

Closes #116

сlose #116

fix: limit the max amount of concurrent network requests

251cf6b

Without limiting the amount of concurrent network requests, installation fails with too many open files error

KSXGitHub reviewed Sep 16, 2023

View reviewed changes

anonrig reviewed Sep 16, 2023

View reviewed changes

KSXGitHub reviewed Sep 17, 2023

View reviewed changes

zkochan added 2 commits September 18, 2023 02:42

Merge branch 'main' into network-concurrency

45c214e

refactor: tarball fetcher

30dcf4f

zkochan added 3 commits September 20, 2023 02:51

Merge branch 'main' into network-concurrency

7cabed4

Merge branch 'main' into network-concurrency

d608dc8

refactor: do changes suggested in PR comments

d65af7f

KSXGitHub mentioned this pull request Sep 20, 2023

feat: panic for programmer error, more diagnostics, remove duplicated map lookup #126

Merged

await-ovo mentioned this pull request Nov 20, 2023

fix(tarball): use rwlock instead of dashmap to avoid potential deadlock #200

Open

KSXGitHub added a commit that referenced this pull request Nov 21, 2023

feat(network): limit concurrent network requests

7fe11cc

Closes #116

KSXGitHub mentioned this pull request Nov 21, 2023

feat(network): limit concurrent network requests #210

Merged

zkochan closed this in #210 Nov 21, 2023

zkochan pushed a commit that referenced this pull request Nov 21, 2023

feat(network): limit concurrent network requests (#210)

744976d

сlose #116

KSXGitHub deleted the network-concurrency branch November 21, 2023 15:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: limit the max amount of concurrent network requests #116

fix: limit the max amount of concurrent network requests #116

zkochan commented Sep 16, 2023

github-actions bot commented Sep 16, 2023 •

edited

Loading

KSXGitHub Sep 16, 2023 •

edited

Loading

KSXGitHub commented Sep 16, 2023

KSXGitHub Sep 16, 2023

zkochan commented Sep 16, 2023

anonrig left a comment

anonrig Sep 16, 2023

KSXGitHub commented Sep 17, 2023

KSXGitHub commented Sep 17, 2023

zkochan commented Sep 17, 2023

KSXGitHub commented Sep 17, 2023

zkochan commented Sep 17, 2023

KSXGitHub Sep 17, 2023

zkochan Sep 20, 2023

KSXGitHub commented Sep 17, 2023 •

edited

Loading

KSXGitHub commented Sep 19, 2023

KSXGitHub commented Sep 19, 2023

zkochan commented Sep 19, 2023

KSXGitHub commented Sep 20, 2023

zkochan commented Sep 20, 2023

fix: limit the max amount of concurrent network requests #116

fix: limit the max amount of concurrent network requests #116

Conversation

zkochan commented Sep 16, 2023

github-actions bot commented Sep 16, 2023 • edited Loading

Benchmark Results

Linux

KSXGitHub Sep 16, 2023 • edited Loading

Choose a reason for hiding this comment

KSXGitHub commented Sep 16, 2023

KSXGitHub Sep 16, 2023

Choose a reason for hiding this comment

zkochan commented Sep 16, 2023

anonrig left a comment

Choose a reason for hiding this comment

anonrig Sep 16, 2023

Choose a reason for hiding this comment

KSXGitHub commented Sep 17, 2023

KSXGitHub commented Sep 17, 2023

zkochan commented Sep 17, 2023

KSXGitHub commented Sep 17, 2023

zkochan commented Sep 17, 2023

KSXGitHub Sep 17, 2023

Choose a reason for hiding this comment

zkochan Sep 20, 2023

Choose a reason for hiding this comment

KSXGitHub commented Sep 17, 2023 • edited Loading

KSXGitHub commented Sep 19, 2023

KSXGitHub commented Sep 19, 2023

zkochan commented Sep 19, 2023

KSXGitHub commented Sep 20, 2023

zkochan commented Sep 20, 2023

github-actions bot commented Sep 16, 2023 •

edited

Loading

KSXGitHub Sep 16, 2023 •

edited

Loading

KSXGitHub commented Sep 17, 2023 •

edited

Loading