Bitmessage/PyBitmessage-2024-12-09
Archived
2
0
Fork 0
Code Issues 168 Pull Requests 53 Actions Packages Projects Releases 5 Wiki Activity

Properly handle SIGTERM if daemon ran with -d #1060

Merged
g1itch merged 3 commits from daemon into v0.6 2018-01-25 03:11:18 +01:00
Conversation 12 Commits 3 Files Changed 4 +10 -7
g1itch commented 2017-09-28 17:18:08 +02:00 (Migrated from github.com)
Copy Link

There is a minor problem: if no daemon setting in keys.dat pybitmessage ignores SIGTERM. I suggest to check singleistance.daemon variable instead of config to determine daemon status.

There is a minor problem: if no `daemon` setting in `keys.dat` pybitmessage ignores `SIGTERM`. I suggest to check `singleistance.daemon` variable instead of config to determine daemon status.
PeterSurda commented 2017-09-30 09:24:04 +02:00 (Migrated from github.com)
Copy Link

Looks ok but I have some other pending stuff to do first. Afterwards I'll ask you to rebase. You can also fix other places where daemon is checked since you're on it.(shutdown, tr, singlecleaner).

Looks ok but I have some other pending stuff to do first. Afterwards I'll ask you to rebase. You can also fix other places where daemon is checked since you're on it.(shutdown, tr, singlecleaner).
f97ada87 commented 2017-09-30 21:47:24 +02:00 (Migrated from github.com)
Copy Link

Good catch, and sorry for the regression that I've caused.
I think the "flake" commits belong in a separate PR as they are not relevant to this bugfix (SIGTERM handling). Other than that, it looks good.

Tested successfully on Linux and an ACK from me (for commit 2a94647 only).

Good catch, and sorry for the regression that I've caused. I think the "flake" commits belong in a separate PR as they are not relevant to this bugfix (SIGTERM handling). Other than that, it looks good. Tested successfully on Linux and an ACK from me (for commit 2a94647 only).
PeterSurda commented 2017-10-02 07:56:18 +02:00 (Migrated from github.com)
Copy Link

Please separate flake8 commits to a different pull request.

Please separate flake8 commits to a different pull request.
g1itch commented 2017-10-02 16:24:44 +02:00 (Migrated from github.com)
Copy Link

There is also shared.daemon introduced in c89d86a: I'm unsure maybe it should be used instead of shared.thisapp.daemon?

There is also `shared.daemon` introduced in c89d86a: I'm unsure maybe it should be used instead of `shared.thisapp.daemon`?
f97ada87 commented 2017-10-02 19:34:23 +02:00 (Migrated from github.com)
Copy Link

I wondered about this too. There is currently a bunch of runtime variables scattered between shared, state and thisapp which seem to be the result of several incomplete migrations.

EDIT (sorry, misread your comment):
Based on my reading of the current code, shared.daemon is the legacy variable currently in use, while shared.thisapp.daemon should never be used outside of main.
Short-term, I suggest that shared.daemon is used across the board at this stage, instead of shared.thisapp.daemon. Long-term I think this should be migrated to state.daemon - as a separate PR.
/EDIT

FTR shared.daemon was not introduced in c89d86a, it was just moved a few lines further down. I avoided changing too much of the status quo to keep the diff small, but it seems I did a pretty poor job at that.

I wondered about this too. There is currently a bunch of runtime variables scattered between `shared`, `state` and `thisapp` which seem to be the result of several incomplete migrations. EDIT (sorry, misread your comment): Based on my reading of the current code, `shared.daemon` is the legacy variable currently in use, while `shared.thisapp.daemon` should never be used outside of `main`. Short-term, I suggest that `shared.daemon` is used across the board at this stage, instead of `shared.thisapp.daemon`. Long-term I think this should be migrated to `state.daemon` - as a separate PR. /EDIT FTR `shared.daemon` was not introduced in c89d86a, it was just moved a few lines further down. I avoided changing too much of the status quo to keep the diff small, but it seems I did a pretty poor job at that.
f97ada87 commented 2017-10-02 19:51:45 +02:00 (Migrated from github.com)
Copy Link

ad0119f tested OK on Linux with daemon=false and --daemon switch.

ad0119f tested OK on Linux with daemon=false and --daemon switch.
g1itch commented 2017-10-06 22:50:54 +02:00 (Migrated from github.com)
Copy Link

Actually I have a problem with desktop app in module tr with this code. Investigating...

Actually I have a problem with desktop app in module `tr` with this code. Investigating...
g1itch commented 2018-01-21 14:28:29 +01:00 (Migrated from github.com)
Copy Link

Is something wrong with this PR?

Is something wrong with this PR?
PeterSurda commented 2018-01-24 20:31:03 +01:00 (Migrated from github.com)
Copy Link

Please rebase, I'm ready to merge, looks ok to me.

Please rebase, I'm ready to merge, looks ok to me.
PeterSurda commented 2018-01-24 20:33:09 +01:00 (Migrated from github.com)
Copy Link

If you want, you can also get rid of shared.daemon, or I can do it after merging this. This includes removing one line in bitmessagemain.py and replacing shared.daemon with shared.thisapp.daemon in class_singleCleaner.py.

If you want, you can also get rid of `shared.daemon`, or I can do it after merging this. This includes removing one line in `bitmessagemain.py` and replacing `shared.daemon` with `shared.thisapp.daemon` in `class_singleCleaner.py`.
PeterSurda commented 2018-01-25 03:13:17 +01:00 (Migrated from github.com)
Copy Link

I think I figured out how to merge without rebasing in a way that doesn't distrupt the workflow, tip4commit and gpg signatures. Should make it easier to merge in the future.

I think I figured out how to merge without rebasing in a way that doesn't distrupt the workflow, tip4commit and gpg signatures. Should make it easier to merge in the future.
PeterSurda commented 2018-01-25 03:13:56 +01:00 (Migrated from github.com)
Copy Link

@g1itch let me know if you'll get rid of shared.daemon (in a separate PR) or want me to do that.

@g1itch let me know if you'll get rid of shared.daemon (in a separate PR) or want me to do that.
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
Labels
Clear labels   
bug

Something isn't working as it's supposed to

  
build

Related to building or build system

  
dependencies

Pull requests that update a dependency file

  
developers

The issue is relevant mainly for developers rather than users

  
documentation

   
duplicate

   
enhancement

New feature

  
formatting

Code or UI text formatting

  
invalid

   
legal

   
mobile

Frontend for mobile devices (kivy)

  
obsolete

   
packaging

   
performance

   
protocol

Protocol change

  
question

   
refactoring

   
regression

Some issue, fixed in the past, appears again

  
security

   
test

   
translation

   
usability

   
wontfix
No Label
bug
build
dependencies
developers
documentation
duplicate
enhancement
formatting
invalid
legal
mobile
obsolete
packaging
performance
protocol
question
refactoring
regression
security
test
translation
usability
wontfix
Milestone
Clear milestone
No items
No Milestone
v0.6.3
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: Bitmessage/PyBitmessage-2024-12-09#1060
No description provided.
Delete Branch "daemon"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?