Skip to content

Don't use ext-readline handler on Mac to fix CR/LF issues on Mac only #73

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 2, 2018

Conversation

clue
Copy link
Owner

@clue clue commented Jul 2, 2018

Resolves / closes #66
Effectively reverts #63 on Mac

@clue clue added this to the v2.2.0 milestone Jul 2, 2018
@clue clue merged commit a62268d into clue:master Jul 2, 2018
@clue clue deleted the readline-mac branch July 2, 2018 17:47
@mkopinsky
Copy link

This is causing some weird issues. When I exit (by hitting Ctrl-D), it outputs PHP Warning: posix_isatty(): supplied resource is not a valid stream resource in /Users/kopinsky/code/botman-playground/vendor/clue/stdio-react/src/Stdio.php on line 338, and doesn't reset the terminal mode.
If I var_dump(STDIN) right before that line, it outputs resource(1) of type (Unknown).

I will try to poke at this a bit later and open a PR (it makes more sense for the guy with a Mac to poke at), but wanted to bring to your attention in case you already know what the issue or fix is.

@clue
Copy link
Owner Author

clue commented Jul 3, 2018

@mkopinsky Thank you for giving this a try and reporting back! I agree and any input would be very much appreciated, as I don't have a Mac to test against 👍 I can not reproduce any issues on my Ubuntu system, maybe we can also come up with a way to test this specific behavior?

@mkopinsky
Copy link

I can try to write a unit test as part of the PR. Seems that travis has a Mac OS environment so we can have that test run on Mac-only.

@clue
Copy link
Owner Author

clue commented Jul 3, 2018

Improving the test suite in this regard would be much appreciated! That being said, our latest tries with Travis' Mac builds weren't all that good (reactphp/stream#120 and others).

@mkopinsky
Copy link

mkopinsky commented Jul 5, 2018

I haven't quite been able to figure it out, but I wanted to share what I'm seeing.

  • When I close by hitting ^D, Stdio::restoreTtyMode() doesn't do its thing, because....
  • Stdio::isTty() returns false, because...
  • STDIN is no longer a valid resource, because ...
  • It was already closed, most likely from Readline::close()...
  • Which gets called from Readline::handleEnd(),
  • which gets called from the c0 event handler on Readline.php:79.

Readline::handleEnd() emits an 'end' event before calling close(), but close() runs first. I suspect that if the order of execution were changed that would fix it, but I'm not familiar enough with the eventemitter stuff to figure out (yet) how to do that.

If I comment out the fclose on vendor/react/stream/src/ReadableResourceStream.php:117 that does fix the behavior of restoreTty, and if I throw an exception there, the stacktrace confirms that it's being called from Readline::close().

@clue
Copy link
Owner Author

clue commented Jul 5, 2018

Thank you for looking into this! I currently fail to see how the stream resource could be closed before invoking the restoreTtyMode() method (which is invoked multiple times just in case).

The following is a gist of what the Stdio class does, so maybe this helps reproducing this:

<?php

use React\Stream\ReadableResourceStream;
use React\EventLoop\Factory;

require __DIR__ . '/../vendor/autoload.php';

$loop = Factory::create();
$stdin = new ReadableResourceStream(STDIN, $loop);

$old = shell_exec('stty -g');
shell_exec('stty -icanon -echo');

$stdin->on('data', function ($chunk) use ($stdin) {
    echo wordwrap(bin2hex($chunk), 2, ' ', true) . PHP_EOL;

    if (strpos($chunk, "\x04") !== false) {
        echo '[EOF]';
        $stdin->close();
    }
});
$stdin->on('close', function () use ($old) {
    echo '[closed]';
    shell_exec('stty ' . $old);
});

$loop->run();

@mkopinsky
Copy link

With that mini script, it worked fine.

$ stty -a
speed 9600 baud; 21 rows; 150 columns;
lflags: icanon isig iexten echo echoe -echok echoke -echonl echoctl
	-echoprt -altwerase -noflsh -tostop -flusho pendin -nokerninfo
	-extproc
iflags: -istrip icrnl -inlcr -igncr ixon -ixoff ixany imaxbel iutf8
	-ignbrk brkint -inpck -ignpar -parmrk
oflags: opost onlcr -oxtabs -onocr -onlret
cflags: cread cs8 -parenb -parodd hupcl -clocal -cstopb -crtscts -dsrflow
	-dtrflow -mdmbuf
cchars: discard = ^O; dsusp = ^Y; eof = ^D; eol = <undef>;
	eol2 = <undef>; erase = ^?; intr = ^C; kill = ^U; lnext = ^V;
	min = 1; quit = ^\; reprint = ^R; start = ^Q; status = ^T;
	stop = ^S; susp = ^Z; time = 0; werase = ^W;
$ php mini.php 
61
73
64
66
04
[EOF][closed]$ stty -a
<snipped - identical to above>

@clue
Copy link
Owner Author

clue commented Jul 6, 2018

That's strange. Can you try patching

public function handleCloseInput()
and add a call to restoreTtyMode() at the start of the method?

@clue
Copy link
Owner Author

clue commented Jul 7, 2018

@mkopinsky I think I've been able to track this down and managed to reproduce this on any system without ext-readline. Can you see if #74 fixes your issues? 👍

@clue
Copy link
Owner Author

clue commented Sep 1, 2018

For the reference: This PR has been reverted via #80 now that #79 addresses the underlying issues. This means that ext-readline will now be used again on all platforms that support it, including Mac OS X.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pressing return in PHPStorm terminal (macOS) doesn't work
2 participants