Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

fix: race condition causing Database is not open error #1834

Merged
merged 1 commit into from
Jan 21, 2019

Conversation

alanshaw
Copy link
Member

@alanshaw alanshaw commented Jan 21, 2019

cleanup closes the DB but yargs-promise does not wait for async stuff in the command handler to finish, and executes that promise chain immediately after the handler is executed. So it's a race condition. In windows, sometimes, the database is closed before the async stuff from the handler completes.

This PR changes the CLI command handlers to always pass a promise to resolve function that yargs-promise adds to the context (argv). This makes yargs-promise wait for it to be resolved before continuing the promise chain and closing the DB.

Since I had to edit all of the commands to make them use the resolve function and introduce promises, I decided to take the opportunity to refactor the CLI commands to use async/await. It should help towards #1670.

Also, w00t! - removed almost 200 LOC

@ghost ghost assigned alanshaw Jan 21, 2019
@ghost ghost added the status/in-progress In progress label Jan 21, 2019
@alanshaw
Copy link
Member Author

@achingbrain this is basically the change you did here for js-ipfs-mfs

@alanshaw alanshaw mentioned this pull request Jan 21, 2019
[`cleanup`](https://github.com/ipfs/js-ipfs/blob/7d9b006d0b0542651cbaa540d5f22a0112ae09bd/src/cli/bin.js#L109) closes the DB but `yargs-promise` does not wait for async stuff in the command handler to finish, and executes that promise chain immediately after the handler is executed. So it's a race condition. In windows, _sometimes_, the database is closed before the async stuff from the handler completes.

This PR changes the CLI command handlers to always pass a promise to `resolve` function that `yargs-promise` adds to the context (`argv`). This makes `yargs-promise` wait for it to be resolved before continuing the promise chain and closing the DB.

Since I had to edit all of the commands to make them use the `resolve` function and introduce promises, I decided to take the opportunity to refactor the CLI commands to use async/await. It should help towards #1670.

License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

🎉

@vasco-santos
Copy link
Member

I will update the DHT CLI for ipfs/js-ipfs#856 according to the changes here!

@alanshaw alanshaw merged commit 6066c97 into master Jan 21, 2019
@ghost ghost removed the status/in-progress In progress label Jan 21, 2019
@alanshaw alanshaw deleted the fix/db-is-not-open branch January 21, 2019 14:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants