Wait for Xpra server to write 'xpra is ready' to stderr before continuing

[ruipin]

Added a "collectOutput" parameter to the subprocessExtras.callBackground function, which pipes stdout/stderr. Using this functionality, the x11Bridge.py waitForServerContainerToLaunch method can now listen and wait until the Xpra server outputs 'xpra is ready' to stderr, before continuing normally.

After these changes, I am unable to reproduce issue #224, and everything else seems to work fine.

Note: I am new at this open-source contribution thing, so feel free to give advice if I did anything wrong :)

[timthelion]

Wow! You did a great job of diving into one of the more hairy parts of the code base and getting things done. Thanks for the pull request!

I did make a couple changes:

• https://github.com/subuser-security/subuser/commit/aa417ae6e663138b8a543787b69bc5ace77ebebb
• https://github.com/subuser-security/subuser/commit/486959a08be697a0f77c9dc1755fce7a1af6fe4d

I wonder if it wouldn't be better to get rid of the suppressOutput code all together and just always collect output (perhaps reprinting it for debugging if necessary).

[ruipin]

Thanks. It wasn't too hard, the code-base isn't that unwieldy. There are some weird things in the code, though, which made it somewhat more complicated, but what took me the longest was to get used to Python again, I haven't done anything with it in years (I prefer Ruby).

Just a small information regarding one of your changes: I left the "debug" outputs there because on first boot, the Xpra server takes something like 15 seconds on my VM to launch, and if I have Xpra debug turned off the application just seems to have frozen. I think it makes complete sense to keep the user up-to-date on what is happening. Maybe print something like "Starting ..." for each runtime instead?

Also, regarding your last point, I agree, it would make sense to remove supressOutput completely. I didn't do it because I didn't want to change too much for a single issue like this. Does Docker support logging to a file in addition to stdout/err? If so, reprinting might not even be necessary.

[timthelion]

I don't want to print to stdout or stderr on subuser run because I want subuser run to be transparent, so that piping the output of subuser programs works. That's why I removed your print statements. However, it just occured to me, that it is possible to test if stdout is a tty, and only print status messages if it is. Therefore, I have re-included the messages with this commit: https://github.com/subuser-security/subuser/commit/6bd62b9d1a56ad8312f266f1122d017db9789b8a

I'd like to improve the code base, what did you find to be weird about it?

I wouldn't use any built in Docker mechanism to deal with the logging. With your collect output option, output is suppressed by default. The only problem is that it is then piped into a pipe, which could become a memory leek in the future because if nothing reads from the pipe, than it will fill up. I don't think it is leeking now, because I don't think that xpra doesn't spew output after it's initial startup.

[ruipin]

I'd like to improve the code base, what did you find to be weird about it?

Well, nothing much, just some stuff like the execute functions sometimes returning a PID (now a supprocess), sometimes a return value was a bit confusing at first. I thought maybe it'd make more sense to always return a subprocess and have the caller grab the return value from there. Not standardizing return values makes it much easier to make mistakes later, as well.

Other than that, it's mostly just my lack of experience with Python. I've gotten so used to Ruby, that Python is really weird!

don't want to print to stdout or stderr on subuser run because I want subuser run to be transparent, so that piping the output of subuser programs works.

Makes sense. The new version seems to work fine, and is much better :)

The only problem is that it is then piped into a pipe, which could become a memory leek in the future because if nothing reads from the pipe, than it will fill up.

I did consider that, actually. Worse, depending on the OS, once the buffer is full, the application writing to the pipe will block until something is read from it. While not too serious (as you say, Xpra doesn't output to stderr other than during the initialization) I've been thinking on how to solve it.

A solution would be redirecting stdout/err to temporary files (tempfile.TemporaryFile), and then reading from it with something like:

while 1:
    where = file.tell()
    line = file.readline()
    if (not line) or (line[-1:] != '\n'):
        time.sleep(0.1)
        file.seek(where)
    else:
        print line,