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

Concurrency Support for Registry Caching #1211

Merged
merged 2 commits into from
Jun 24, 2021
Merged

Conversation

JakeCooper
Copy link
Contributor

@JakeCooper JakeCooper commented Jun 17, 2021

Summary

If the pack command is run concurrently and registry caches are constructured, there is a racecase which occurs where both invocations will attempt to update the fs

Output

Before

file exists ERROR: failed to build: locating in registry heroku/nodejs-yarn@0.1.3: refreshing cache: initializing (<$HOME>/.pack/registry-a932275bd19c2d9e1b88fa06698fd2f5427a363d25bf87fa500691c373089381): creating registry cache: rename /tmp/registry373937049 <$HOME>/.pack/registry-a932275bd19c2d9e1b88fa06698fd2f5427a363d25bf87fa500691c373089381: file exists

This change captures and passes on that error

After

Documentation

  • Should this change be documented?
    • Yes, see #___
    • No

Related

Resolves #___

@JakeCooper JakeCooper requested a review from a team as a code owner June 17, 2021 01:34
@github-actions github-actions bot added the type/enhancement Issue that requests a new feature or improvement. label Jun 17, 2021
@github-actions github-actions bot added this to the 0.20.0 milestone Jun 17, 2021
@aemengo
Copy link
Contributor

aemengo commented Jun 17, 2021

Thanks so much for making a contribution!

I'm a little uneasy however about validating on a naked string like that. I'd be more inclined to check if the file is already there, but the code already does that...

May I ask the use-case for running this pack command concurrently?

@JakeCooper
Copy link
Contributor Author

JakeCooper commented Jun 17, 2021

Totally understand. Ideally the os package would provide enums for it's errors but I couldn't find them!

EDIT: It appears the package does export an error enum so I've updated the PR :)

We use pack to power the buildpack experience for http://railway.app/. When a user fires builds without dockerfiles, we use pack to bundle their code to docker images.

We had a user today use an older version of a buildpack which fired two builds using the same repository cache, resulting in a collision in the file system

@aemengo
Copy link
Contributor

aemengo commented Jun 17, 2021

Looks good to me. I think it's up to a maintainer to give their seal of approval.

I wish you the best of luck with http://railway.app/. Looks really cool!

@jromero jromero added type/bug Issue that reports an unexpected behaviour. and removed type/enhancement Issue that requests a new feature or improvement. labels Jun 17, 2021
@JakeCooper
Copy link
Contributor Author

Bump. Any thoughts here or things I'd need to do to get this merged?

CC @jromero

Signed-off-by: Jake Cooper <jake.elijah.cooper@gmail.com>
Signed-off-by: Jake Cooper <jake.elijah.cooper@gmail.com>
@github-actions github-actions bot added the type/enhancement Issue that requests a new feature or improvement. label Jun 24, 2021
Copy link
Member

@dfreilich dfreilich left a comment

Choose a reason for hiding this comment

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

Looks great to me, and thank you for the PR! Always majorly appreciated when users of pack contribute fixes, in addition to error reports :D

You da man

dfreilich added a commit that referenced this pull request Jun 24, 2021
Polish #1211 - Add Concurrency support
Signed-off-by: David Freilich <freilich.david@gmail.com>
@dfreilich dfreilich merged commit 1f34bd0 into buildpacks:main Jun 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug Issue that reports an unexpected behaviour. type/enhancement Issue that requests a new feature or improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants