feat(mongodb-downloader)!: use a lockfile to prevent redundant parallel downloads MONGOSH-1875 #580
Conversation
gagik
changed the title
| Object.create(null); | ||
|
|
||
| // Download mongod + mongos and return the path to a directory containing them. | ||
| async downloadMongoDbWithVersionInfo( |
There was a problem hiding this comment.
I know this is kind of a big breaking change... I am open to refactoring back but this should relatively simple migration generally and in my opinion a general improvement.
There was a problem hiding this comment.
I see the lockfile part as also a big "breaking" change (unless we make it opt-in) so I kind of see an opportunity to do this now
There was a problem hiding this comment.
I guess I'm not really clear on what we get out of changing the public interface of this package?
Like, the class as a code organization tool is perfectly fine, but if we're already worrying about cross-process download interactions (which we should!), is it still worth exposing this to users of this package as a way to worry about in-process download interactions?
There was a problem hiding this comment.
yeah that is totally fair re: public interface. The original motivation for changing the public interface came from the initial approach which was "locking" in-memory inside the class so then the instance being managed would be up to the user. Obviously with the file-based approach this is irrelevant.
So I'll keep the class refactor for sake of code organization if that sounds good but keep the old API.
There was a problem hiding this comment.
Re: opting in/out,
opt-in seems safest in terms of not breaking workflows so I'll do this for now. I can make this a required property so projects can decide and discover this functionality easily. so it'd be a breaking change but very explicitly about this.
gagik
force-pushed
the
gagik/download-dir
branch
from
fa16082 to
20e4664
Compare
| // Adapted from: | ||
| // https://raw.githubusercontent.com/npm/cli/072253549d774893a3689341dbc660cb845ebcfe/workspaces/libnpmexec/lib/with-lock.js | ||
|
|
||
| // The Artistic License 2.0 |
There was a problem hiding this comment.
There was a problem hiding this comment.
ah, that's a good doc to have on hand. Welp, I guess I'll use proper-lockfile
| const controller = new AbortController(); | ||
| let release: (() => Promise<void>) | undefined; | ||
|
|
||
| const lockFile = `${originalFilePath}/.mongodb-downloader-lock`; |
There was a problem hiding this comment.
this is annoying but actually proper lockfile expects an existing file to create a lock around, so we'll have both .mongodb-downloader-lock and .mongodb-downloader-lock.lock...
so this dummy file actually does nothing, it's .mongodb-downloader-lock.lock that is relevant.
I can't do it on i.e. mongod since it's existence is used to figure out if the file is actually downloaded. alternatively, I could create a dummy mongod with some flag to it to differentiate
| }); | ||
| }); | ||
|
|
||
| describe('version name', function () { |
There was a problem hiding this comment.
I tried to untangle the version resolution stuff and adding these to make sure this is the expected output
This is making use of the new locking mechanism with the runner and downloader in mongodb-js/devtools-shared#580 as well as having a single place for all binaries.
This is making use of the new locking mechanism with the runner and downloader in mongodb-js/devtools-shared#580 as well as having a single place for all binaries.
2 participants
Should help in cases where multiple processes are trying to download Mongo binaries.