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

make child and parent die together #24

Merged
merged 1 commit into from
Jul 19, 2017

Conversation

nikanar
Copy link

@nikanar nikanar commented Jul 18, 2017

Same as ActivityWatch/aw-watcher-window#20 for other watcher.

Aims to solve ActivityWatch/aw-qt#19

Watcher process checks for being an orphan, and if it is, breaks.

Attempt at using branches, looking good, amazingly.

Copy link
Member

@johan-bjareholt johan-bjareholt left a comment

Choose a reason for hiding this comment

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

Looking good :)

Same here that you didn't commit with the same user as you made the PR with, but that's fine for me.

Great work!

@@ -59,7 +58,7 @@ def ping(self, afk):
data = {"status": "afk" if afk else "not-afk"}
e = Event(data=data, timestamp=self.now)
self.client.heartbeat(self.bucketname, e, pulsetime=self.settings.timeout, queued=True)

Copy link
Member

Choose a reason for hiding this comment

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

Trailing whitespace, but it's just one so it's fine.

@johan-bjareholt johan-bjareholt merged commit 5888188 into ActivityWatch:master Jul 19, 2017
@ErikBjare
Copy link
Member

The removed newlines should have been kept (PEP8 recommends 2 lines between top level functions and classes, so strict linters will complain).

Just one issue: This likely won't work with PyInstaller since it creates two Python processes afaik. One that's some kind of "bootloader" and the other which actually hosts the watcher (it's the bootloader that will be orphaned, not the watcher itself). This will lead to this not working in the PyInstaller case, but perhaps there is a way to detect PyInstaller and work around it.

Nice work regardless, this solution never occurred to me.

@nikanar
Copy link
Author

nikanar commented Jul 20, 2017

@ErikBjare Let's discuss this PyInstaller problem in a new issue maybe ? First step would be to see what happens when aw-qt "crashes normally" under the PyInstaller bootloader - do the other processes still survive as in OP of #19 ? (so PyInstaller creates 2 instead of 1, but AW is 4, so how many total processes are we talking about here, 5 or 8 ? How is the PGID / process group thing set then ? etc.) I don't know how PyInstaller works, but I could imagine it only being the father of the aw-qt process, letting aw-qt father the others, and when aw-qt dies, maybe aw-server's PID becomes PyInstaller's instead of aw-qt's (then we would have to detect a change, rather than a 1 value).

Newlines : duly noted, my bad.

@ErikBjare
Copy link
Member

I continued the discussion here: ActivityWatch/aw-qt#19

@nikanar nikanar deleted the die-together branch July 20, 2017 17:55
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.

3 participants