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

ignore stale API files and deprecate ipfs repo fsck #6478

Merged
merged 4 commits into from
Jul 3, 2019
Merged

Conversation

Stebalien
Copy link
Member

  1. Users now never need to run ipfs repo fsck (and, if they do, it'll do nothing).
  2. Stale "api" files are ignored if the daemon isn't running and the repo is initialized.

This means the following finally works reliably:

ipfs id
ipfs daemon &
sleep 2
killall -9 ipfs
sleep 2
ipfs id

@Stebalien Stebalien requested review from Kubuxu and djdv June 29, 2019 10:09
This command is no longer necessary and is quite dangerous:

1. All lockfiles are now released by the OS when the daemon stops.
2. The API file is ignored when (a) the repo is initialized and (b) daemon is
off.

fixes #6435
cmd/ipfs/main.go Outdated
exctr = cmds.NewExecutor(req.Root)
// Force the daemon when the API flag is passed (unless we're trying to
// _run_ the daemon).
daemonForced := apiAddr != nil && req.Command != daemonCmd
Copy link
Contributor

Choose a reason for hiding this comment

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

pedantic nits:

Force the daemon when the API flag is passed

Should probably be more specific here, forcing the daemon to what?
Initialize temporarily?

daemonForced -> daemonRequested
Despite a user supplying an API addr, commands don't aways force the daemon to do anything.
It's requested, and can be denied if the request is incompatible.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, updated. I went back and forth on that several times but decided that e.g., ipfs version should just be run on the client regardless.

Copy link
Contributor

@djdv djdv left a comment

Choose a reason for hiding this comment

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

LGTM
Small and optional pedantic comment made in PR.

In the future, (if we can) it may be good to create the API file in the same way we do for the repo lockfile. (have the OS treat it as a temporary file that is removed when the process is no longer running).
But this PR is still necessary for platforms that don't support such a feature, and can contain stale API files.

@Stebalien
Copy link
Member Author

In the future, (if we can) it may be good to create the API file in the same way we do for the repo lockfile. (have the OS treat it as a temporary file that is removed when the process is no longer running).
But this PR is still necessary for platforms that don't support such a feature, and can contain stale API files.

At this point, we assume that all platforms support it (in practice, all do). The issue is that the daemon may be running on a separate machine.

@Stebalien Stebalien merged commit eb4da3c into master Jul 3, 2019
@Stebalien Stebalien deleted the fix/stale-api branch July 3, 2019 21:30
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.

2 participants