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

bugfix: double delete of pidfile #470

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

PorcelainMouse
Copy link

Removed redundant os.remove(options.pidfile) at daemon_entry.py:136. cmp to daemon.py:165.

Removed redundant os.remove(options.pidfile).
Copy link
Member

@cas-- cas-- left a comment

Choose a reason for hiding this comment

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

I'm afraid this is not the right solution. Can you please provide more details on the bug you are encountering?

This is a user command line supplied arg pidfile perhaps the path supplied matches the daemon's internal pidfile? get_config_dir('deluged.pid'). It is possible the file was deleted before daemon exited?

I suspect the more correct solution here is a try block around the remove statement to catch file not found errors.

@PorcelainMouse
Copy link
Author

That doesn't surprise me. I can't tell what is the difference between these two internal variables. I just guessed that they were different names for the same thing. If that's not right, then my patch isn't right.

I think your first suggestion is correct. I do specify a pidfile in the config directory.
... --pidfile=%h/.config/deluge/deluged.pid ...

Well, the file is clearly being deleted before the last attempt to remove it, but the file absolutely exists when I quite the daemon. That's the first thing I checked.

In reading the docs, I didn't realize that the --pidfile would not override the "internal pidfile" definition. I think that is the expected behavior of such a command line parameter, and that my interpretation is consistent with the manpage. Understanding this better now, I would modify my suggested solution to also de-duplicating these two PID file variables when the command line parameter is used/parsed. That is, either change the logic so that there is only one pidfile, and drop the extra delete code, or change the logic to ensure the two pidfiles never point to the same path, and then leave the code I tried to remove as it is. Does that make sense? What do you think about that idea?

@cas--
Copy link
Member

cas-- commented Feb 10, 2025

The options.pidfile is not the same as the internal one and will cause issues with detecting running daemon if it's overwritten since the internal version is a combination of pid;port. I know that before this option was added there have been quite a few discussions around this file, it's name and it's contents.

Sure we could have a check, although might be tricky since argparserbase is common code vs daemon-specific pidfile location. Alternatively an update to the docs to avoid creating files that clash with existing files in config directory.

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