You are not logged in.

#1 2021-01-20 07:03:18

johnraff
nullglob
From: Nagoya, Japan
Registered: 2015-09-09
Posts: 12,557
Website

Possible occasional race condition running jgmenu-gtktheme.py?

There has been a small number of reports, going back months, of jgmenurc being truncated so jgmenu reverts to the default, eg https://forums.bunsenlabs.org/viewtopic … 20#p105120
I hit the same issue myself a couple of times, usually when running the gtk or ob sync commands, but it's very intermittent and I haven't found a way of reproducing it reliably.

But it happened again three days ago, I did some more poking around in the code and wondered if it wasn't jgmenu-gtktheme.py? My Python knowlege is almost zero, but it seemed that the script is calling jgmenu_run config -s file -k key -v value multiple times, for each key-value pair that is edited. Considering the ~250ms that the script runs in (on my machine) that might be a longish window for other processes to try to write to ~/.config/jgmenu/jgmenurc?

I don't know if that has any bearing on the jgmenurc corruption issue or not, but wondered if changing the algorithm slightly might be a Good Thing anyway?
Like this:

cp jgmenurc jgmenurc~tmp~
do all the edit calls on jgmenurc~tmp~
mv jgmenurc~tmp~ jgmenurc

Just as a Proof of Concept I hacked a modified script that works that way. Please don't just use it as-is: there's no error checking (eg that the temp file has really been created) and I gathered that os.system is not considered the best way to do this kind of thing anyway, but it does seem to work:

#!/usr/bin/env python3

# Copyright (C) @Misko_2083 2019
# Copyright (C) Johan Malm 2019

""" Parse gtk theme and set some key/value pairs in jgmenurc """

import os
import sys
import shlex
import subprocess
try:
    import gi
except ImportError:
    print("[gtktheme] fatal: require python3-gi")
    sys.exit(1)

gi.require_version("Gtk", "3.0")
from gi.repository import Gtk

def run(command):
    """ run a command """
    subprocess.Popen(shlex.split(command))

def fmt(s):
    """ ensure string is at least two characters long """
    if len(s) == 0:
        return "00"
    elif len(s) == 1:
        return "0{}".format(s)
    return s

def rgb2hex(line):
    """ convert rgb values to a 6-digit hex string """
    s = line.split("rgb(")
    rgb = s[-1].replace(");", "").split(",")
    r = hex(int(rgb[0]))[2:]
    g = hex(int(rgb[1]))[2:]
    b = hex(int(rgb[2]))[2:]
    return "{}{}{}".format(fmt(r), fmt(g), fmt(b))

def setconfig(key, value):
    """ set key/value pain in ~/.config/jgmenu/jgmenurc """
#    filename = os.environ["HOME"] + "/.config/jgmenu/jgmenurc"

    value = "#{} 100".format(value)
    print("[gtktheme] {} = {}".format(key, value))
    cmd = "jgmenu_run config -s {} -k {} -v '{}'".format(tempfile, key, value)
    run(cmd)

def process_line(line):
    """ process one line """
    if "background-color" in line:
        setconfig("color_menu_bg", rgb2hex(line))
        setconfig("color_title_bg", rgb2hex(line))
        setconfig("color_title_border", rgb2hex(line))

def cache(themename):
    """ save the theme-name to ~/.cache/jgmenu/.last-gtktheme """
    print("themename={}".format(themename))
    directory = os.environ["HOME"] + "/.cache/jgmenu"
    if not os.path.exists(directory):
        os.mkdir(directory)
    f = open(directory + "/.last-gtktheme", "w")
    f.write(themename)
    f.close()

def main():
    """ main """
    gset = Gtk.Settings.get_default()
    themename = gset.get_property("gtk-theme-name")
    cache(themename)
    prefdark = gset.get_property("gtk-application-prefer-dark-theme")
    css = Gtk.CssProvider.get_named(themename).to_string()
#    print(css)
#    exit(1)
    lines = css.split("\n")

    global tempfile
    filename = os.environ["HOME"] + "/.config/jgmenu/jgmenurc"
    tempfile = filename + "~tmp~"
    cpcmd = "cp " + filename + " " + tempfile
    os.system(cpcmd)

    # parse some @define-color lines
    for line in lines:
        if "@define-color" not in line:
            break
        if "theme_text_color" in line:
            setconfig("color_norm_fg", rgb2hex(line))
            setconfig("color_title_fg", rgb2hex(line))
        if "theme_selected_bg_color" in line:
            setconfig("color_sel_bg", rgb2hex(line))
        if "theme_selected_fg_color" in line:
            setconfig("color_sel_fg", rgb2hex(line))

    # parse the menu { } section
    inside = False
    for line in lines:
        if "menu {" in line:
            inside = True
            continue
        if inside:
            if "{" in line:
                inside = False
                break
            process_line(line)

    mvcmd = "mv " + tempfile + " " + filename
    os.system(mvcmd)

if __name__ == '__main__':
    main()

If jgmenurc is a symlink, the final mv will overwrite it though. sad
I guess a comprehensive rewrite of the script to gather all the changes in an array and write them out in a single operation would be a better way, but you might well feel your time has better things to be devoted to right now...

Last edited by johnraff (2021-01-21 00:35:10)


...elevator in the Brain Hotel, broken down but just as well...
( a boring Japan blog (currently paused), now on Bluesky, there's also some GitStuff )

Introduction to the Bunsenlabs Boron Desktop

Offline

#2 2021-01-20 18:12:23

nobody
The Great
Registered: 2015-08-10
Posts: 3,655

Re: Possible occasional race condition running jgmenu-gtktheme.py?

Which script calls jgmenu-gtktheme.py?

Comments on the code

- I agree that jgmenu should be called as few times as possible but if igmenu config can only set 1 key at a time, that won't work? If jgmenu has direct ways of performing IPC, the best way would be to rewrite jgmenurc and then tell it to reload. It would be great if there was locking in jgmenu or if jgmenu respected locking
- at the beginning of jgmenu-gtktheme, you could create a lockfile in the user's rundir, or take a cooperative lock on some file in order to prevent multiple instances from jgmenu-gtktheme from running at the same time. Additional jgmenu-gtkthemes would then wait until they can get a lock. Technically this could also be retrofitted using a wrapper script with flock(1) and a lockfile specific to that script
- You could run jgmenu-gtktheme using systemd-run --user --wait with a fixed unit name so that only 1 unit is active at any time, but this would only work if it's always called that way
- if jgmenurc is a symlink, use os.path.realpath to resolve the symlink and os.path.abspath to get the absolute path of the target file, then operate only on that file
- the approach to create a tempfile first and then call rename(2) is the correct way to overwrite a file, in Python, you should use https://docs.python.org/3/library/shuti … hutil.move instead of a shellout, it will do the most difficult things around rename(2) for you

Offline

#3 2021-01-20 22:24:33

malm
jgmenu developer
Registered: 2016-10-13
Posts: 735
Website

Re: Possible occasional race condition running jgmenu-gtktheme.py?

I’ll have to write back another day, because it’ll take me a while and I’m too tired.

Offline

#4 2021-01-21 00:36:56

johnraff
nullglob
From: Nagoya, Japan
Registered: 2015-09-09
Posts: 12,557
Website

Re: Possible occasional race condition running jgmenu-gtktheme.py?

(OP: Fixed incorrect linking, and repeatedly called command is jgmenu_run config -s file -k key -v value.)


...elevator in the Brain Hotel, broken down but just as well...
( a boring Japan blog (currently paused), now on Bluesky, there's also some GitStuff )

Introduction to the Bunsenlabs Boron Desktop

Offline

#5 2021-01-24 16:33:33

malm
jgmenu developer
Registered: 2016-10-13
Posts: 735
Website

Re: Possible occasional race condition running jgmenu-gtktheme.py?

johnraff wrote:

My Python knowlege is almost zero, but it seemed that the script is calling jgmenu_run config -s file -k key -v value multiple times, for each key-value pair that is edited. Considering the ~250ms that the script runs in (on my machine) that might be a longish window for other processes to try to write to ~/.config/jgmenu/jgmenurc?

Yes, jgmenu-gtktheme.py calls `jgmenu_run config` multiple times.
I wasn't aware of the bug reported in the OP, but have managed to reproduce it.
Having just looked at jgmenu-gtktheme.py, it calls the subprocess asynchronously, which is not right, so have pushed a commit to "subprocess.Popen.wait()".

The startup and hook functions both use g_spawn_command_line_sync(), so we
shouldn't have multiple jgmenu-gtktheme.py processes running even if the user kept clicking to open/close jgmenu.

Given the above, I think it's pretty unlikely that other processes would be writing to jgmenurc whilst jgmenu-gtktheme.py is running. Please challenge me on this if you think I'm not considering it properly. I'm happy to implement flock(2) or lockf(3) - if we can convince ourselves that there is a need.

nobody wrote:

I agree that jgmenu should be called as few times as possible but if igmenu config can only set 1 key at a time, that won't work? If jgmenu has direct ways of performing IPC, the best way would be to rewrite jgmenurc and then tell it to reload. It would be great if there was locking in jgmenu or if jgmenu respected locking

jgmenu-obtheme.c only reads and writes jgmenurc once.

As jgmenu-gtktheme.py is in python I couldn't easily reuse the code, so just did multiple calls to the `jgmenu_run config` helper. The simplest solution is probably be to let the python script read/parse/write jgmenurc rather than calling `jgmenu_run config`.

Just for reference
- jgmenu uses IPC, but not for setting config variables. It's uses USR1 to write to a self-pipe which is read in the main loop.
- jgmenu uses a lockfile to avoid multiple instances of long-running menu processes.

nobody wrote:

- at the beginning of jgmenu-gtktheme, you could create a lockfile in the user's rundir, or take a cooperative lock on some file in order to prevent multiple instances from jgmenu-gtktheme from running at the same time. Additional jgmenu-gtkthemes would then wait until they can get a lock. Technically this could also be retrofitted using a wrapper script with flock(1) and a lockfile specific to that script

Yes - although thinking this through, I think it's highly unlikely that multiple instances of jgmenu-gtktheme.py will be running. The culprit was the async call to `jgmenu_run config`

Offline

#6 2021-01-25 08:06:25

johnraff
nullglob
From: Nagoya, Japan
Registered: 2015-09-09
Posts: 12,557
Website

Re: Possible occasional race condition running jgmenu-gtktheme.py?

Thanks for looking at this @malm - it looks as if you've put your finger on the issue.

malm wrote:

Given the above, I think it's pretty unlikely that other processes would be writing to jgmenurc whilst jgmenu-gtktheme.py is running. Please challenge me on this if you think I'm not considering it properly. I'm happy to implement flock(2) or lockf(3) - if we can convince ourselves that there is a need.

Not a challenge really, but what about completely outside processes - user scripts etc? Is there perhaps a case for making jgmenu-gtktheme.py's write to jgmenurc as near to atomic as possible? Is there a kind of lock that protects a file from opening by any process, or only for programs that participate in the agreement?

The simplest solution is probably be to let the python script read/parse/write jgmenurc rather than calling `jgmenu_run config`.

And do it's write-out in one step, like jgmenu-obtheme.c?

But since you've succeeded in reproducing the issue I'm sure you'll find the best solution without too much bickering from the sidelines. smile


...elevator in the Brain Hotel, broken down but just as well...
( a boring Japan blog (currently paused), now on Bluesky, there's also some GitStuff )

Introduction to the Bunsenlabs Boron Desktop

Offline

#7 2021-01-26 06:28:24

johnraff
nullglob
From: Nagoya, Japan
Registered: 2015-09-09
Posts: 12,557
Website

Re: Possible occasional race condition running jgmenu-gtktheme.py?


...elevator in the Brain Hotel, broken down but just as well...
( a boring Japan blog (currently paused), now on Bluesky, there's also some GitStuff )

Introduction to the Bunsenlabs Boron Desktop

Offline

Board footer

Powered by FluxBB