ponty / PyVirtualDisplay

Python wrapper for Xvfb, Xephyr and Xvnc
BSD 2-Clause "Simplified" License
709 stars 78 forks source link

Check the display successfully starts before returning (2) #33

Closed Elias481 closed 4 years ago

Elias481 commented 6 years ago

After checking for successfull start it should be verified that the display found running with xdpyinfo is really the display we started. If multiple python processes open new display at the same time it's possible that the same target display number is allocated by multiple instances the same time. First X server process started will successfully come up of course, but the other processes just see the X-Server at that displaynumber running and do not complain. Problem occurs (in our case) when the real owner of the process calls the displays close/shuts down the process. All other processes using this X Server are now getting problems. (Probably problems can arise even before if the script needs a dedicated/unshared X server instance to be successfull.)

I see with random display number I can minimize chances that this happens, but anyway I cannot be sure (without setting MIN_DISPLAY_NUMBER explicitly to some unused range what is quite inconvenient as we would need to assign a unique MIN_DISPLAY_NUMBER to every script running on that server).

Currently I had to implement something like the following to avoid running into the problem, but it would be fine if the API would handle this.

    def start_display(tries, *args, **kwargs):
        display = Display(*args, **kwargs)
        display.start()
        dispstarttry = 1
        while not display.is_alive():
            if dispstarttry >= tries:
                display.wait()
                print('Could start display after %d tries\nXvfb output last try:\nSTDOUT:\n-----------------------\n%s\nSTDERR:\n-----------------------\n%s'
                      % (dispstarttry, display.stdout, display.stderr))
                sys.exit(2)
            display.close()
            display = Display(*args, **kwargs)
            display.start()
            dispstarttry += 1
        return display
ponty commented 4 years ago

"After checking for successfull start it should be verified that the display found running with xdpyinfo is really the display we started."

In latest version the X server process is checked after the display was found with xdpyinfo. The X server process exits if it doesn't have the display.

            if not self.is_alive():
                msg = "Failed to start process: %s"
                raise XStartError(msg % self)
Elias481 commented 4 years ago

@ponty Sadly that is not enough. I found that a sleep of 2 seconds (more then 1 second) was necessary to be reliably enough on our server. Thought that sleep is missing there - an a sleep is never a good solution. So currently we have it stable with

    def start_display(tries, *args, **kwargs):
        display = Display(*args, **kwargs)
        display.start()
        dispstarttry = 1
        time.sleep(2)
        while not display.is_alive():
            if dispstarttry >= tries:
                display.wait()
                print('Could start display after %d tries\nXvfb output last try:\nSTDOUT:\n-----------------------\n%s\nSTDERR:\n-----------------------\n%s'
                      % (dispstarttry, display.stdout, display.stderr))
                sys.exit(2)
            display.stop()
            display = Display(*args, **kwargs)
            display.start()
            dispstarttry += 1
            time.sleep(2)
        return display

Where both is done (ensuring that it is started with a 2 seconds delay and also starting it on error. We would expect the module to also ensure it is started if the only reason is that the insufficent port-allocation algorithm is not working...

ponty commented 4 years ago

The latest version is different:

There is no need for sleep if you have recent Xvfb with -displayfd flag, on Ubuntu this is from 2016.04. on. If you have older Xvfb, then tries is integrated into start() and start() returns only if the process is alive and requested X display is available (using xdpyinfo)

From latest README:

Concurrency

"Recent X servers as of version 1.13 (Xvfb, too) support the -displayfd command line option: It will make the X server choose the display itself" https://stackoverflow.com/questions/2520704/find-a-free-x11-display-number/

First help text is checked (e.g. Xvfb -help) to find if -displayfd flag is available. If -displayfd flag is available then it is used, if not then there are 10 retries by default which should be enough for starting 10 concurrent X servers. The retries parameter can be increased if necessary.

Elias481 commented 4 years ago

ah yes I remeber I think I myself proposed such patch, it was just not really feasible/possible without changing the underlying Popen wrapper back then. So I will check current version and probly close then. thanks that far

Elias481 commented 4 years ago

Yes I see this is now obsolete then. Don't know why I missed the mail regarding implementation of my PR in May 2019 (whis is now mostly revised, but not dropped reagrding content)...