A recent commit I LGTM'd causes unit tests to fail on macOS but not FreeBSD

[krader1961]

I gave a LGTM to commit 8ba041a since building with that change succeeded and gave me a ksh command I could run on macOS and FreeBSD. However, I didn't run the unit tests. I just noticed, as part of testing the fix for issue #7, that those tests fail on macOS but not FreeBSD. The output of bin/package test ast-ksh on macOS begins with the following and not a single test passes:

/Users/krader/projects/3rd-party/ast/arch/darwin.i386-64/bin/ksh: cannot create pipe [Protocol not supported]
package: test output captured in /Users/krader/projects/3rd-party/ast/arch/darwin.i386-64/lib/package/gen/test.out
package: test start at Wed Nov  8 20:59:43 PST 2017 in /Users/krader/projects/3rd-party/ast/arch/darwin.i386-64
++ set -
cmd/INIT:
/Users/krader/projects/3rd-party/ast/arch/darwin.i386-64/src/cmd/INIT
++ regress /Users/krader/projects/3rd-party/ast/src/cmd/INIT/iffe.tst iffe
./regress: line 236: syntax error near unexpected token `('
./regress: line 236: `                  !($KEEP))       j="$j $i" ;;'
make [cmd/INIT]: *** exit code 2 making test.iffe

That first line is probably the important one.

[siteshwar]

Ksh can use sockets to implement pipes. I suggest you to look into this code.

[siteshwar]

I think there are problems with these socket attributes on macOS.

[qbarnes]

I'd suggest being careful with changing to using sockets. The socket interface used on RHEL 7 has been nothing but a broken mess, yet Red Hat refuses to change the ksh default on their system back to pipes out of fear, I was told:

Switching to real pipes instead of sockets will cause performance degradation. Also, despite of all test cases passing, there is still a major risk of regression with real pipes. It would be too risky to switch to real pipes instead of sockets.

Two problems I ran across with ksh on RHEL 7. First one:
$ cat /etc/passwd | head -1 /dev/stdin
head: cannot open ‘/dev/stdin’ for reading: No such device or address

Second one is a some sort of race that reproduces less frequently the more CPUs you have in the system, but locking ksh to one CPU easily shows it:
$ taskset 1 ksh
$ top
top: failed tty set: Interrupted system call

These problems reproduce with the ksh shipping with RHEL 7.4 as well as att/ast version on master branch. I have a lot more detail on both problems. Just ask if curious.

I would love to know if these problems happen on other OSes (Linux and non-Linux) and what the underlying causes are.

[siteshwar]

$ taskset 1 ksh
$ top
top: failed tty set: Interrupted system call

This is not an issue with sockets. It is a race condtion beause ksh uses posix_spawn to fork processes. You can read the upstream discussion around it here.

[qbarnes]

Ah, that's right. That one wasn't a socket vs. pipe problem, but the posix_spawn() race.

I take it there's no answer to Brian Ginn's question about, "Is there a reliable way to for a process to determine if it is running in the background?"

[krader1961]

The AST feature testing in this situation is so wrong it's amazing. Look at this block of code from src/lib/libast/features/fcntl.c:

#ifndef SOCK_CLOEXEC
        printf("#ifndef SOCK_CLOEXEC\n");
        printf("#define _ast_SOCK_CLOEXEC       1\n");
        printf("#define SOCK_CLOEXEC            02000000 /* %s */\n", ORIGIN_EXTENSION);
        printf("#endif\n");
#endif
#ifndef SOCK_NONBLOCK
        printf("#ifndef SOCK_NONBLOCK\n");
        printf("#define _ast_SOCK_NONBLOCK      1\n");
        printf("#define SOCK_NONBLOCK           04000   /* %s */\n", ORIGIN_EXTENSION);
        printf("#endif\n");
#endif

You can't arbitrarily define syscall constants like that! Jeebus H. Christ! Those fallback definitions are why this issue exists. This bogosity was papered over by the code in the recently removed src/lib/libast/port/intercept.c module. The removed socktype() function called fcntl() with the correct flags to emulate those incorrect SOCK_* symbols.

On macOS the correct value for SOCK_CLOEXEC (aka O_CLOEXEC) is 0x1000000, not 0x2000000. Although even that is a hack since you can't, technically, pass O_CLOEXEC to the socket() call. It does, however, work. This has been discussed in other projects such as here: https://www.redhat.com/archives/libguestfs/2015-February/msg00054.html.

[krader1961]

Also, not to derail this issue, but posix_spawn() is a horrible API. AFAICT the performance improvements it provides is nowhere close to justifying its existence. Its use is also the source of some job management bugs in the Fish shell. So the fact it seems to be the source of bugs in ksh does not surprise me.

[krader1961]

It turns out that passing O_CLOEXEC to the socket() call fails on macOS 10.12.6. So even that hack doesn't work. It appears that we need to reinstate the code that was removed which does not rely on SOCK_CLOEXEC.

[siteshwar]

AFAIK ksh uses sockets instead of real pipes due to perfromance reasons. I would like to investigate if we should continue to use this approach or switch to real pipes.

[siteshwar]

test io begins at 2017-11-10+13:33:08                                                                                 
        io.sh[255]: <# not working for pipes               
        io.sh[342]: read -n3 from pipe not working                                                                    
        io.sh[351]: read -n3 from fifo failed -- expected 'a', got 'abc'
        io.sh[354]: read -n1 from fifo failed -- expected 'b', got 'd'                                                
        io.sh[384]: should have timed out                  
        io.sh[385]: line1 should be 'prompt1: '                                                                       
        io.sh[386]: line2 should be line2                  
        io.sh[387]: line3 should be 'prompt2: '                                                                       
        io.sh[411]: LC_ALL=C read -n2 from pipe 'a bcd' failed -- expected 'a bcd', got 'ab cd'
        io.sh[411]: LC_ALL=C.UTF-8 read -n2 from pipe 'a bcd' failed -- expected 'a bcd', got 'ab cd'                 
test io failed at 2017-11-10+13:33:21 with exit code 10 [ 99 tests 10 errors ]
test io(C.UTF-8) begins at 2017-11-10+13:33:21                                                                        
        io.sh[255]: <# not working for pipes                
        io.sh[342]: read -n3 from pipe not working
        io.sh[351]: read -n3 from fifo failed -- expected 'a', got 'abc'
        io.sh[354]: read -n1 from fifo failed -- expected 'b', got 'd'
        io.sh[384]: should have timed out
        io.sh[385]: line1 should be 'prompt1: '
        io.sh[386]: line2 should be line2
        io.sh[387]: line3 should be 'prompt2: '
        io.sh[411]: LC_ALL=C read -n2 from pipe 'a bcd' failed -- expected 'a bcd', got 'ab cd'
        io.sh[411]: LC_ALL=C.UTF-8 read -n2 from pipe 'a bcd' failed -- expected 'a bcd', got 'ab cd'
test io(C.UTF-8) failed at 2017-11-10+13:33:34 with exit code 10 [ 99 tests 10 errors ]

Switching to real pipes gives these test failures. So it looks like real pipes were not well tested.

[krader1961]

Sockets were definitely faster than pipes back in the days of SVR2 and BSD 4.2. Mostly because they used a fixed size 8 KiB kernel buffer to shuttle the data between the read and write endpoints and weren't bidirectional. SVR4 STREAMS pipes were much more performant but obviously never got adopted by other OS's. I have no idea what the situation is today but will try to find out.

The one thing that really annoys me (more than the stupid #define SOCK_CLOEXEC 02000000 is the #define pipe ast_pipe. If an abstraction is needed that hides whether pipes, sockets, or some other mechanism is being used at least give it a distinct name like io_channel. Confusing the reader who sees pipe(fds) into thinking that the pipe syscall is being used is an anti-pattern.

[krader1961]

Also, it's interesting to note that SOCK_CLOEXEC and SOCK_NONBLOCK do not appear in the index of the current edition of the APUE book. So these are apparently fringe features.

[krader1961]

It looks like the performance of pipe() is comparable to socketpair() on some systems (e.g., BSD) and up to 15% slower on others. Whether the additional complexity to detect and use socketpair() is worthwhile is debatable. If we do keep that optimization I'd love to see the code restructured to make it easier to read.