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: attempt to only allow one deno process to update the node_modules folder at a time #18058

Merged

Conversation

dsherret
Copy link
Member

@dsherret dsherret commented Mar 7, 2023

With an artificial delay:

2023-03-07_19-47-16.mp4

This is implemented in such a way that it should still allow processes to go through when a file lock wasn't properly cleaned up and the OS hasn't released it yet (but with a 200ms-ish delay).

Closes #18039

@dsherret dsherret marked this pull request as ready for review March 8, 2023 00:49
@dsherret dsherret requested a review from bartlomieju March 8, 2023 00:50
@@ -255,17 +277,25 @@ impl ProgressBar {
}

pub fn update(&self, msg: &str) -> UpdateGuard {
self.update_with_prompt(ProgressMessagePrompt::Download, msg)
}
Copy link
Member Author

@dsherret dsherret Mar 8, 2023

Choose a reason for hiding this comment

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

I could have spent some time making our progress bars more general use, but I didn't think it was worth the time atm.

Copy link
Member

Choose a reason for hiding this comment

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

As I side note: we should lazily create ProgressBar on ProcState - it currently shows up in the flamegraphs for each startup when it's checking if we can actually draw. (even if we never use the progress bar, like when only loading local files)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll lazily create it here

Copy link
Member Author

@dsherret dsherret Mar 8, 2023

Choose a reason for hiding this comment

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

Oh wait, not worth it because IS_TTY_WITH_CONSOLE_SIZE is already in a lazy and it would have hit that by now.

Edit: Meh, I'll just lazily create it anyway.

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +536 to +545
// Spawn a blocking task that will continually update a file
// signalling the lock is alive. This is a fail safe for when
// a file lock is never released. For example, on some operating
// systems, if a process does not release the lock (say it's
// killed), then the OS may release it at an indeterminate time
//
// This uses a blocking task because we use a single threaded
// runtime and this is time sensitive so we don't want it to update
// at the whims of of whatever is occurring on the runtime thread.
tokio::task::spawn_blocking({
Copy link
Member

Choose a reason for hiding this comment

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

Cool 👍 I was worried about this scenario, but you handled it nicely.

@@ -255,17 +277,25 @@ impl ProgressBar {
}

pub fn update(&self, msg: &str) -> UpdateGuard {
self.update_with_prompt(ProgressMessagePrompt::Download, msg)
}
Copy link
Member

Choose a reason for hiding this comment

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

As I side note: we should lazily create ProgressBar on ProcState - it currently shows up in the flamegraphs for each startup when it's checking if we can actually draw. (even if we never use the progress bar, like when only loading local files)

@dsherret dsherret enabled auto-merge (squash) March 8, 2023 14:53
@dsherret dsherret merged commit 88b5fd9 into denoland:main Mar 8, 2023
kt3k pushed a commit that referenced this pull request Mar 10, 2023
…s folder at a time (#18058)

This is implemented in such a way that it should still allow processes
to go through when a file lock wasn't properly cleaned up and the OS
hasn't released it yet (but with a 200ms-ish delay).

Closes #18039
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.

Improve synchronization of multiple deno processes caching node_modules folder
2 participants