First steps of splitting io_data_t

[xiaq]

First steps towards #484.

By using shared_ptr to manage io_data_t and avoid copying io_data_t, better destruction semantics is achieved. io_buffer_destroy will be converted into a virtual destructor a bit later (after splitting io_buffer_t, of course).

[ridiculousfish]

This looks like a great first step, but I want to be sure we test it carefully.

[xiaq]

@ridiculousfish I haven't written test cases against memory code (fish is the first C++ project I contribute seriously), so I wonder what exactly should be tested against. Perhaps:

• that io_chain_t::duplicate{,prepend} does copy the pointer instead of the real thing? I think declaring the copy constructor of io_data_t private eliminates the need for such tests.
• proper destruction of io_data_t stored in io_chain_t?
• or... some hints?

[ridiculousfish]

By testing, I mean integration testing. Here's an example of a problem I introduced when I tried to clean this stuff up before (in commit 61686af): #269 . It popped up only when using fish_config with tmux - yikes.

After review, here's one potential source of trouble I spotted. In postfork.cpp : free_redirected_fds_from_pipes(), we try to handle the case where a redirection using an fd above 2 (a feature used by the eval function) collides with the fd returned by pipe(). Previously, this would not affect the redirections used by the job, because it modified a copy; but now the change may affect the values seen by other commands in the pipeline. (The handle_child_io call is not a concern, because it is called after fork, but the call within free_redirected_fds_from_pipes is a potential concern). It may be that a job_t's io_chain never contains IO_PIPE redirections, in which case this would not be real.

Also, the pipe_read and pipe_write variables in exec.cpp : exec, will no longer be copied, and it's hard for me to reason about what the consequences of that are, because they are used in a strange way inside the function. It would be nice

The last suggestion I have is to prefer to pass shared_ptrs by const reference.

Overall, if this change proves correct, it will be an excellent improvement and I'll be thrilled. I'm going to merge it so we can get testing and exposure.

[ridiculousfish]

Thank you!

[ridiculousfish]

It appears that commit 8b10b0a broke my prompt (which is simple):

function fish_prompt
    set_color $fish_color_cwd
    echo -n (basename $PWD)
    set_color normal
    echo -n ' ) '
end

The color no longer appears. This most likely means that the output from builtins is not being correctly interleaved with the output from external commands.

I am going to back out this change until we have a fix.

[xiaq]

@ridiculousfish That's weird, it works for me here. No regression in make test either.

[ridiculousfish]

A reduced test case is:

echo (command echo 1; echo 2)

this should output '1 2', but instead it outputs '2 1'. Tested on Ubuntu and OS X.

[xiaq]

@ridiculousfish Ah, this is interesting! I'm looking into it.

[ridiculousfish]

Reverted here: 77f1b1f

[xiaq]

I've locked down the bug to the behavior change of io_chain_t::duplicate_prepend, called from exec.cpp:exec:

    if (! parser.block_io.empty())
    {
        io_duplicate_prepend(parser.block_io, j->io);
    }

When I restore the copy behavior, echo (command echo 1; echo 2) outputs 1 2.

But after looking through the code for quite a while, I still don't have a clue. @ridiculousfish What do you suspect may be causing this?

[JanKanis]

My usual approach for these kinds of problems is to step through it in a
debugger. That will (nearly) always tell you what's going on if the bug is
deterministic. Did you try that?
On Jan 5, 2013 11:25 AM, "xiaq" notifications@github.com wrote:

I've locked down the bug to the behavior change of
io_chain_t::duplicate_prepend, called from exec.cpp:exec:

if (! parser.block_io.empty())
{
    io_duplicate_prepend(parser.block_io, j->io);
}

When I restore the copy behavior, echo (command echo 1; echo 2) outputs 1
2.

But after looking through the code for quite a while, I still don't have a
clue. @ridiculousfish https://github.com/ridiculousfish What do you
suspect may be causing this?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/487#issuecomment-11912457.

[xiaq]

@JanKanis ok, I'll try a debugger. The tricky part is where to break at...

[JanKanis]

I'd say to start at where you made the critical change.
On Jan 5, 2013 2:07 PM, "xiaq" notifications@github.com wrote:

@JanKanis https://github.com/JanKanis ok, I'll try a debugger. The
tricky part is where to break at...

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/487#issuecomment-11913755.

[ridiculousfish]

I speculate it's due to this code at the bottom of exec.cpp:exec:

for (io_chain_t::const_iterator iter = parser.block_io.begin(); iter != parser.block_io.end(); iter++)
{
    io_remove(j->io, *iter);
}

This removes the IO redirections in parser.block_io from the job, by comparing literal pointer values. When we copied, we got different pointers; now that we are no longer copying we retain the existing pointers, so we may be removing more.

It's possible that code never actually did anything in fish 1.x, because the redirections were always copied, so nothing was ever removed. But I want to make sure we understand what that code does and why, or convince ourselves that it does nothing, before we modify it. IO redirections are one of the most mysterious parts of fish (at least to me).

[xiaq]

@ridiculousfish Ha, removing the removing fixes it. :) I guess it affects the behavior of job_continue, I'll read the code.

Also, I'd bet that it did nothing before, but I'll double check.

[xiaq]

Update:

• I've confirmed that removing IO_BUFFERs messes up the execution of job_continue - more specifically, the select_try and read_try calls in it. I'm still not 100% sure about the mechanism (still plowing through the code), but I'd say that getting rid of the removing is the correct fix.
• Also I have confirmed that the removing didn't really remove anything. Wrapping the loop like this breaks nothing:

    size_t n0 = j->io.size();

    for (io_chain_t::const_iterator iter = parser.block_io.begin(); iter != parser.block_io.end(); iter++)
    {
        io_remove(j->io, *iter);
    }

    if (n0 != j->io.size())
        throw "";

[xiaq]

It seems that:

• Normally the writes into a IO_BUFFER is picked up in job_continue with a select_try loop, calling read_try when the select succeeds.
• There is a call to io_buffer_read in exec_subshell_internal. Why it is needed is a mystery to me, since the above procedure should have read all the outputs.
• When the IO_BUFFERs are removed, outputs are no longer read in job_continue. Instead, they are all picked up by io_buffer_read called from exec_subshell_internal. And this results in the wrong order - output from builtins appear before output from externals. That's another mystery.

[xiaq]

I've further confirmed that the io_buffer_read in exec_subshell_internal doesn't read anything under most circumstances. The following change against master branch has no visible impact with my casual test.

diff --git a/exec.cpp b/exec.cpp
index 6794fb5..65ebf0d 100644
--- a/exec.cpp
+++ b/exec.cpp
@@ -1453,7 +1453,11 @@ static int exec_subshell_internal(const wcstring &cmd, wcstring_list_t *lst)
         status = proc_get_last_status();
     }

+    size_t n0 = io_buffer->out_buffer_size();
     io_buffer_read(io_buffer);
+    size_t n1 = io_buffer->out_buffer_size();
+    if (n1 != n0)
+        printf("Got %lu -> %lu\n", n0, n1);

     proc_set_last_status(prev_status);