Skip to content

Remove uncommon functions from common module #17781

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.

Sign up for GitHub

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

Closed
Trott wants to merge 4 commits into from
Closed

Remove uncommon functions from common module #17781

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
69eb221
test: remove common.projectDir
Trott Dec 20, 2017
19bd327
test: remove getTTYfd() from common module
Trott Dec 20, 2017
cfa5ae0
test: move firstInvalidFD() out of common module
Trott Dec 20, 2017
8be1259
test: fix buggy getTTYfd() implementation
Trott Dec 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
test: remove getTTYfd() from common module 
common.getTTYfd() is used in one test only. Move it's definition to that
test and out of the common module.
  • Loading branch information
@Trott
Trott committed Dec 20, 2017
commit 19bd327eb2ef1cac4cf4a879016914c3a05d180f
17 changes: 0 additions & 17 deletions test/common/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -823,23 +823,6 @@ exports.crashOnUnhandledRejection = function() {
(err) => process.nextTick(() => { throw err; }));
};

exports.getTTYfd = function getTTYfd() {
const tty = require('tty');
let tty_fd = 0;
if (!tty.isatty(tty_fd)) tty_fd++;
else if (!tty.isatty(tty_fd)) tty_fd++;
Copy link
Copy Markdown
Member

@lpinca lpinca Dec 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this branch ever reached?

Copy link
Copy Markdown
Member

@lpinca lpinca Dec 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nvm, 8be1259 is the answer.

else if (!tty.isatty(tty_fd)) tty_fd++;
else {
try {
tty_fd = fs.openSync('/dev/tty');
} catch (e) {
// There aren't any tty fd's available to use.
return -1;
}
}
return tty_fd;
};

// Hijack stdout and stderr
const stdWrite = {};
function hijackStdWritable(name, listener) {
Expand Down
19 changes: 18 additions & 1 deletion test/sequential/test-async-wrap-getasyncid.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,24 @@ if (common.hasCrypto) { // eslint-disable-line crypto-check

{
// Do our best to grab a tty fd.
const tty_fd = common.getTTYfd();
function getTTYfd() {
const tty = require('tty');
let tty_fd = 0;
if (!tty.isatty(tty_fd)) tty_fd++;
else if (!tty.isatty(tty_fd)) tty_fd++;
else if (!tty.isatty(tty_fd)) tty_fd++;
Copy link
Copy Markdown
Member

@TimothyGu TimothyGu Dec 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can someone explain to me how this works?

Copy link
Copy Markdown
Member

@benjamingr benjamingr Dec 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't write the code but I assume:

The default file descriptors for I/O are stdin, stdout and stderr (0, 1 and 2 respectively). It's not always guaranteed to be the case, so we try the first 3 and then try /dev/tty which usually works (it maps for the controlling terminal of a process for each process) but it's slower to call than isatty it if it's 0.

Copy link
Copy Markdown
Member Author

@Trott Trott Dec 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was a cut-and-paste, I didn't write it, but yeah, it's not just hard to follow, but also buggy. If the first if fails, on line 253, it increments tty_fd but doesn't then check the condition on line 254 because that's an else so it skips that line. Ugh.

If 0 and 1 are not TTYs but 2 is, we won't get 2 (unless that's what's returned by the try/catch block subsequently).

I think something more like this is what we want:

let tty_fd = [0, 1, 2].find(tty.isatty);
if (tty_fd === undefined) {
     try {
        tty_fd = fs.openSync('/dev/tty');
      } catch (e) {
        // There aren't any tty fd's available to use.
        return -1;
      }
}
return tty_fd;

(And while I'm in there fixing this function so it hopefully works correctly, I should probably refactor tty_fd to have a name that doesn't include _.)

Copy link
Copy Markdown
Member Author

@Trott Trott Dec 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a commit here to fix this. PTAL.

else {
try {
tty_fd = fs.openSync('/dev/tty');
} catch (e) {
// There aren't any tty fd's available to use.
return -1;
}
}
return tty_fd;
}

const tty_fd = getTTYfd();
if (tty_fd >= 0) {
const tty_wrap = process.binding('tty_wrap');
// fd may still be invalid, so guard against it.
Expand Down