PICARD-2471: Restrict Picard to a single instance

[skelly37]

Summary

• This is a…
  • Bug fix
  • Feature addition
  • Refactoring
  • Minor / simple change (like a typo)
  • Other
• Describe this change in 1-2 sentences:

This is a draft where we can discuss my GSoC project progress. Picard is going to work in single-instance mode thanks to named pipes

Problem

Ticket description is clear enough so I'll just paste it here:

When passing folders to Picard via command line (such as in Foobar), Picard will launch a new instance for each folder rather than loading them all in one instance. The ability to force it all into one instance would be helpful.

• JIRA ticket (optional): PICARD-2471

Solution

I've created a cross-platform communication protocol for Picard instances, based on named pipes. Now, in this draft, you can see my progress in integrating it into the Picard app.

TODO

• Communication protocol works on all supported operating systems
• Pipe can distinguish between app versions
• If pipe is listened to, Picard sends all the arguments there and leaves without spawning an instance
• Picard accepts -s/--stand-alone-instance argument to create a forced, standalone instance, that doesn't listen to any pipe
• Picard creates a "pipe server" thread that listens to any possible messages in the background and passes messages to file-adding methods
• in tagger.py/main: picard_args.FILE args are converted to absolute paths before passing them to the existing instanec
• Changes are tested
• Changes are documented
• Commits are properly organised
• Merge :)

Notes

• Type annotations are my helpers, since I always use mypy while writing Python. When we will be merging into master, I can remove them if they are problematic for you
• Commit structure is also just a draft to be organised properly
• Because it's just a draft, I'll let you know when workflows will make sense.

[skelly37]

I made a terrible mistake of not creating a proper branch when doing my initial PR. Actual work on the ticket starts with bab2ee7

[zas]

I made a terrible mistake of not creating a proper branch when doing my initial PR. Actual work on the ticket starts with bab2ee7

Rebase your patch of latest master branch to clear out unwanted commits please.

[skelly37]

Okay, commits should be good too

[phw]

Not sure what's wrong with the tests, that's odd. There is not even a test run started, and I can't seem to manually start it.

But there is also a merge conflict since I merged #2127 that should be resolved. Not sure if this could be related? Maybe Github does not run the pipeline for unmergable PRs?

[skelly37]

I was thinking about it a bit, how about making Pipe.is_pipe_owner False when FileNotFoundError, and in tagger.py replace while True with while pipe_handler.is_pipe_owner?

It should close the listener when the file is deleted and picard will be able to exit correctly.

[skelly37]

Thanks :)

And I thank you for the merge :)

[phw]

I was thinking about it a bit, how about making Pipe.is_pipe_owner False when FileNotFoundError, and in tagger.py replace while True with while pipe_handler.is_pipe_owner?

It should close the listener when the file is deleted and picard will be able to exit correctly.

Please try that. But I'm still not 100% sure where it actually hangs. On exit it sets self.pipe_running to False in tagger.py, so to my understanding the pipe listener already should stop there because in pipe_server there is while self.pipe_running. But maybe if we change this to an attribute of the Pipe instance and set this to False on FileNotFoundError will indeed solve the issue.

[skelly37]

There's a progress but it's still unexitable, see:

D: 10:09:05,274 /usr/lib/python3.10/site-packages/picard/tagger.exit:419: Picard stopping
I: 10:09:05,274 /usr/lib/python3.10/site-packages/picard/browser/browser.stop:134: Stopping the browser integration
D: 10:09:05,378 /usr/lib/python3.10/site-packages/picard/webservice/ratecontrol.decrement_requests:146: ('picard.musicbrainz.org', 443): Decrementing requests to: 0
E: 10:09:05,378 /usr/lib/python3.10/site-packages/picard/webservice/__init__._handle_reply:516: Network request error for https://picard.musicbrainz.org/api/v2/releases: Operation canceled (QT code 5, HTTP code 0)
QIODevice::read (QNetworkReplyHttpImpl): device not open
E: 10:09:05,378 /usr/lib/python3.10/site-packages/picard/util/checkupdate._releases_json_loaded:103: Error loading Picard releases list: Operation canceled
D: 10:09:05,378 /usr/lib/python3.10/site-packages/picard/ui/mainwindow.update_last_check_date:1924: The update check was unsuccessful. The last update date will not be changed.
D: 10:09:05,378 /usr/lib/python3.10/site-packages/picard/webservice/ratecontrol._out_of_backoff:222: ('picard.musicbrainz.org', 443): oobackoff; delay: 1000ms -> 1000ms; slow start; window size 1.000 -> 2.000

[skelly37]

@rdswift you might also want to take a look at the new help message

[zas]

There's a progress but it's still unexitable

When is this "unexitable" issue happening? In which situation(s)?

[skelly37]

There's a progress but it's still unexitable

When is this "unexitable" issue happening? In which situation(s)?

As @phw mentioned, when you delete the pipe file yourself, Picard becomes unexitable, as you can see in the logs.

Basically, the PipeErrorNotFound doesn't stop the program for some reason.

[phw]

@skelly37 zas and I discussed this a bit. Let's get this PR merged as it is (after some final cleanup and closing of open discussions, e.g. the open discussion about the help output). We can tackle the closing issue separately. I think it should be handled, but it is not essential. Even if we don't find a proper solution it would be ok to just note this as a known issue. We can still try to fix this, though ;)

We should also discuss how you continue from this point on. This turned out to be much easier to handle cross-platform than anticipated, also you cam into this project so well prepared :D

You could help with some of the refactorings we want to do for Picard 3, or maybe we find some tasks related to this one (such as PICARD-2519, but this is a smaller thing, or maybe we have ideas on other ways this instance communication can be utilized). We also briefly discussed if there is some value to use DBus as the primary way of handling this single instance issue on platforms where this is available by default (Linux desktops mostly), and fall back to the pipe if not. Not really sure what the benefits would be and if it is worse the effort. I had some initial work done into this direction a while back, but abandoned it due to the limited support for other platforms.

I'd suggest we discuss this on IRC, so we can get something interesting here. You probably have some ideas yourself ;)

[skelly37]

We can still try to fix this, though ;)

True, let’s open an issue with this when the PR will get merged

We should also discuss how you continue from this point on. This turned out to be much easier to handle cross-platform than anticipated, also you cam into this project so well prepared :D

You could help with some of the refactorings we want to do for Picard 3, or maybe we find some tasks related to this one (such as PICARD-2519, but this is a smaller thing, or maybe we have ideas on other ways this instance communication can be utilized). We also briefly discussed if there is some value to use DBus as the primary way of handling this single instance issue on platforms where this is available by default (Linux desktops mostly), and fall back to the pipe if not. Not really sure what the benefits would be and if it is worse the effort. I had some initial work done into this direction a while back, but abandoned it due to the limited support for other platforms.

Hm, initially we were talking about better error prompts for users but the mentioned issues also seem to be interesting. Picard3 refactors and CLI enhancements like the URLs should be fine to start with, imo :)

I'd suggest we discuss this on IRC, so we can get something interesting here. You probably have some ideas yourself ;)

Agreed, tho I’m low on time until Monday, I’ll reach you on Monday.

[phw]

Agreed, tho I’m low on time until Monday, I’ll reach you on Monday.

Actually that fits me well

[skelly37]

Actually, give me 1h before merge, I might've come to a solution

edit: nevermind, it didn't work out, you can merge