-
Notifications
You must be signed in to change notification settings - Fork 8
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
Reconsider the use of stacker
in remove_recursively()
#516
Comments
In terms of filesystem limitations... On Unix, there typically isn't a filesystem based limitation, however it seems One exception to the above would be symlinks - via either loops, or else just extending the depth by referring to directory containing a deep directory structure, from an already deep directory. However in the recursive deletion case, we're not following symlinks by design, so this is not an issue. On Windows, the max path limit is 260 characters, which means even with single character directory names, the practical limit is 130 directories deep. In the common case for CNBs the contents of the layers are going to be generated by cross-platform compatible tools, which will have to adhere to the Windows depth limits. However just for completeness I tested against the Unix limits instead. I removed the use of $ git diff -U6
diff --git a/test-buildpacks/readonly-layer-files/src/layer.rs b/test-buildpacks/readonly-layer-files/src/layer.rs
index 7beb18d..a94b6f6 100644
--- a/test-buildpacks/readonly-layer-files/src/layer.rs
+++ b/test-buildpacks/readonly-layer-files/src/layer.rs
@@ -26,13 +26,14 @@ impl Layer for TestLayer {
fn create(
&self,
_context: &BuildContext<Self::Buildpack>,
layer_path: &Path,
) -> Result<LayerResult<Self::Metadata>, <Self::Buildpack as Buildpack>::Error> {
let directory = layer_path.join("sub_directory");
- fs::create_dir_all(&directory)?;
+ let deep_subdirectory = directory.join("a/".repeat(2000));
+ fs::create_dir_all(&deep_subdirectory)?;
fs::write(directory.join("foo.txt"), "hello world!")?;
// By making the sub-directory read-only, files inside it cannot be deleted. This would
// cause issues when libcnb.rs tries to delete a cached layer directory unless libcnb.rs
// handles this case explicitly. Running the test resulted in the buildpack build succeeding (ie no stack overflow) - however lifecycle failed as it can't handle paths that long:
Trying again using a Therefore:
As such, we don't need to use |
Since: - The stdlib `remove_dir_all` recursive implementation (which this function is replacing) doesn't have stack overflow protection - Testing has proven it's unnecessary (see #516) In addition: - The function has been refactored to avoid recursing for every file (now only directories), which reduces depth of recursion by one, and saves an additional stat on every file, since the file type is already retrieved by `read_dir` so it's redundant to call the more expensive `symlink_metadata()` again on the next recursion. - `remove_recursively` has been renamed to `remove_dir_recursively` to more clearly reflect it's intended to operate on directories (and for closer parity to the name of `remove_dir_all`) GUS-W-11917787.
Since: - The stdlib `remove_dir_all` recursive implementation (which this function is replacing) doesn't have stack overflow protection - Testing has proven it's unnecessary (see #516) In addition: - The function has been refactored to avoid recursing for every file (now only directories), which reduces depth of recursion by one, and saves an additional stat on every file, since the file type is already retrieved by `read_dir` so it's redundant to call the more expensive `symlink_metadata()` again on the next recursion. - `remove_recursively` has been renamed to `remove_dir_recursively` to more clearly reflect it's intended to operate on directories (and for closer parity to the name of `remove_dir_all`) GUS-W-11917787.
In #488 the
stacker
crate was added as a dependency, since there was a concern about stack overflows, given the addedremove_recursively()
function is recursive.The explanation for adding this can be seen in the code comment:
libcnb.rs/libcnb/src/util.rs
Lines 44 to 51 in 109ba19
However:
remove_dir_all
implementation (which this function replaces) is also recursive, and it doesn't protect against stack overflows.Whilst the dependency is only small (so the impact on compile times likely not significant), we should still avoid unnecessary dependencies where possible, since in aggregate it can make a difference. In addition, it seems worth avoiding the cognitive burden of this uncommon dependency, if we don't actually need to use it.
The text was updated successfully, but these errors were encountered: