Line prefix issues post-#715 #745

Merged
merged 4 commits into from Oct 4, 2012

Projects

None yet

2 participants

@bitprophet
Member

In #715 the io loops were refactored to be a lot faster. Unfortunately this necessitated tweaking newline detection and it seems to have broken re: line prefix in some situations, for example (serial execution -- not parallel):

[app02.prod] Executing task 'srsly'                             
[app02.prod] sudo: rm -rf /mnt/services/neon /mnt/services/warp9
[app02.prod] out: sudo password:                                

[app02.prod] out: [app08.prod] Executing task 'srsly'           
[app08.prod] sudo: rm -rf /mnt/services/neon /mnt/services/warp9
[app08.prod] out: sudo password:                                
[app08.prod] out: [app09.prod] Executing task 'srsly'           
[app09.prod] sudo: rm -rf /mnt/services/neon /mnt/services/warp9
[app09.prod] out: sudo password:                                
[app09.prod] out: [app10.prod] Executing task 'srsly'           
[app10.prod] sudo: rm -rf /mnt/services/neon /mnt/services/warp9
[app10.prod] out: sudo password:                                
[app10.prod] out: [s0063.prod] Executing task 'srsly'           
[s0063.prod] sudo: rm -rf /mnt/services/neon /mnt/services/warp9
[s0063.prod] out: sudo password:                                
[s0063.prod] out:                                               

Notice how two line prefixes "pile up" on each other on about half these lines.

May be limited to this specific situation, where a single newline of stdout from one host triggers a newline + prompt, then the next host's "Executing" line prints its prompt + text + newline.

This pattern seems to describe stdout from a run/sudo ending with a trailing newline; anything following that will be part of a different loop context (or, as with "Executing" lines, not part of any IO loop but simply a print()) and unable to detect that something has printed to the local stdout + not tied off with a newline.

Offhand fix: ensure termination of the IO loop always ends with a newline being printed to local stdout?

cc @octplane

@octplane
octplane commented Oct 3, 2012

Hum,

after a bit of thinking, I wonder if https://github.com/fabric/fabric/blob/master/fabric/io.py#L174 is not the one responsible for the missing \n: this line sends a \n to the remote and usually in interactive console, the \n is also taken in account, thus generating an extra carriage return.

In our case, the \n is never echoed locally, hence the missing carriage return...

We could also ensure all io loop always end with a newline, which is not completely silly after all (I guess some remote could not end with a newline at all and we would sensibly add a newline, just for cleanness sake)...

Have a test case ? :-)

@bitprophet
Member

I've been hacking on this the last two days, and what I'm arriving at is:

  • Keep not only a read, but also a write, buffer, including our line prefixes (just for state keeping)
  • Inspect that buffer at the end of the loop and print a newline if the last thing in the loop is the line prefix

Or some variation on that theme. I was trying to detect the "ended with a line prefix w/o any newline after it" state without such a write-buffer, and it doesn't work due to the 'editing' we do of the read data. (I.e. the read buffer does not match what we actually write, due to some newline chomps and so on.)

Hopefully keeping effectively 2x the remote stdout around in memory won't be too expensive in nontrivial situations. Could easily make this output buffer a circular one though.


You might be right about the non-echoed "\n" too, @octplane, since so far I've only reproduced the problem like so:

@hosts([2-3 hosts here])
def mytask():
    run("sudo -k") # clear remote sudo password caching
    sudo("rm -f /nonexistent/directory")

Which results in the same kinds of output as I pasted in the description.

I tried using e.g. run("echo -n blah") but that appears to work correctly somehow, though I didn't dig into it.

@octplane I'm fine taking the lead on this but would like you to help verify the fix once it's pushed :)

@bitprophet
Member

Think I got it. @octplane let me know what you think about the changes, but I'm happy and will probably merge this soon :)

@octplane
octplane commented Oct 4, 2012

I like that ! (Oh my god what an amount of work you're providing, while I'm drowning doing scala :/)...

@bitprophet bitprophet merged commit f9817ac into master Oct 4, 2012

1 check passed

Details default The Travis build passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment