Single instance mode(Addresses #324)

[merrittlj]

Multiple instances of bemenu running at once can freeze/not grab the keyboard once the first instance is closed. This PR kills other bemenu instances when the most recent instance grabs the keyboard, as multiple instances are most likely accidental(need to spam the hotkey or similar, as described by issue #342) and not helped by bemenu's design.

This can be disabled and reverted back to old behavior via the "--multi-instance-mode" flag.

Closes issue #342.

[merrittlj]

A more elegant solution for forcing a single instance may be present, but I saw that the main impact of multiple instances was in interference during keyboard grabbing(errors "x11: cannot grab keyboard" with multiple instances without this commit).

Optionally, I could resolve this issue via preventing an instance from launching if one is already present, if that is preferable.

[Earnestly]

I do have a script which runs two bemenus in parallel using both -f and --ifne. Will this change mean I am forced to break my pipelines up and store intermediate data in files?

Oh I just noticed you're trying to do process management on PIDs that are not children of bemenu itself:

snprintf(buffer, sizeof(buffer), "pgrep bemenu-run | grep -v %d | xargs -r kill", this_pid);

Absolutely not, nevermind the extremely poor use of pgrep, grep and xargs to do this.

[merrittlj]

I don't understand what you are trying to say. I will most likely try to improve this to prevent extra instances spawning on startup rather than killing them after. If necessary, I can add an option to disable this prevention once it is implemented.

[merrittlj]

Can you elaborate on the poor use? I'm not the most experienced with some of these tools.

[merrittlj]

I am working on implementing a improved solution to this, do not merge this yet and disregard the above details concerning the previous solution.

[Cloudef]

1. I think this should be part of common.c (the cli programs) and not the library unless given new API
2. Use flock instead, something like:

   int lock_fd = open("/var/run/bemenu.lock", O_CREAT | O_RDWR | O_CLOEXEC, 0666);
   errno = 0;
   if (flock(lock_fd, LOCK_EX | LOCK_NB) != 0) {
       errx(EXIT_FAILURE, "bemenu instance is already running");
   }

   then you do not close this fd, it will be closed by OS when the process dies, this should be race free AFAIK

[merrittlj]

Got it. I was planning on doing something similar to what you proposed.

[merrittlj]

The code is now updated with an improved solution, with help and suggestions from @Earnestly and @Cloudef . It can optionally be disabled/reverted back to the previous behavior with the command line flag "--multi-instance-mode".

[Cloudef]

About the --multi-instance-mode flag, either drop it all together. Or make this new feature under a flag like --single-instance. I don't myself see a need for flag at all but what does @Earnestly think?

[merrittlj]

I believe we should have single instance mode as the default(multiple instances can be confusing to a new user), and have it as toggleable, as while it may not be immediately useful for some, some people may want to take advantage of multiple instances in some way.

In addition, there's no need to remove user control or options, as they don't have any detrimental impact staying in.

[Cloudef]

In that case I would just not have flag at all. Otherwise it's opt-in to the old behavior. There's already way too many flags and possible combinations.

[merrittlj]

Do you believe we should reenable multiple instance support later on if it is improved in some way(such as correctly focusing the second instance after the first one closes) to where it is more practical?

[merrittlj]

I personally support the idea mentioned by @Earnestly in the issue connected to this pull request. We can leave this pull request open for now, but I can move my focus to better support for multiple instances, rather than avoiding it via a forced single-instance mode.

[Cloudef]

Maybe change the flag to --single-instance and we can merge this. This way it does not break existing behavior but can still be used. Regarding the last post on the issue, focus controlling may be impossible under wayland, and even then bemenu really won't know the opening order, that would need a daemon.

[merrittlj]

Okay, I see. I will make this change when I can. I can experiment with multiple instance support, but with the issues you mentioned, it may not be possible(I am unable to test on platforms such as Wayland, anyways).

[merrittlj]

@Cloudef Should be ready to merge, changed to a flag to enable single-instance behavior.

[Cloudef]

Looks good, can you also add the option to the man page.

[merrittlj]

man page has been updated, should be ready to merge.

[Cloudef]

Thanks