Skip to content
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

lib,src: Ensure '(node:pid)' prefix for stdout logging #3833

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
10 participants
@JungMinu
Copy link
Member

commented Nov 15, 2015

Fixes issue: #3789
add '(node:pid)' prefix message for stdout logging

@martfors

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2015

If there aren't any reason template string can't be used in these files maybe this would be a good time to do that switch for these lines?

@thefourtheye

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2015

I would recommend moving this to function in internal/util and use template strings as suggested by @martfors

@JungMinu

This comment has been minimized.

Copy link
Member Author

commented Nov 15, 2015

@thefourtheye will do :)

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Nov 15, 2015

The square brackets don't really add anything useful. I would format it as (node:1234), it's shorter.

@JungMinu

This comment has been minimized.

Copy link
Member Author

commented Nov 15, 2015

@bnoordhuis Thanks :)

@JungMinu JungMinu changed the title lib: Ensure '(node) [pid]' prefix for stdout logging lib: Ensure '(node:pid)' prefix for stdout logging Nov 15, 2015

@silverwind

View changes

lib/_debugger.js Outdated
@@ -1641,7 +1643,7 @@ Interface.prototype.trySpawn = function(cb) {
process._debugProcess(pid);
} catch (e) {
if (e.code === 'ESRCH') {
console.error(`Target process: ${pid} doesn't exist.`);
console.error(prefix + pid + `Target process: ${pid} doesn't exist.`);

This comment has been minimized.

Copy link
@silverwind

silverwind Nov 15, 2015

Contributor

Careful here, pid is defined in this inner scope, overshadowing your top-scope definition. Won't be an issue when you use a function, though :)

This comment has been minimized.

Copy link
@JungMinu

JungMinu Nov 15, 2015

Author Member

@silverwind Yup, I will use a function :)

@thefourtheye

View changes

lib/internal/util.js Outdated
@@ -1,6 +1,7 @@
'use strict';

const prefix = '(node) ';
const node_pid = process.pid;
const prefix = `(node:${node_pid}) `;

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Nov 15, 2015

Contributor

You can simply do const prefix = (node:${process.pid}) ;

@thefourtheye

View changes

lib/internal/util.js Outdated
return exports._printStdoutError(msg);
};

exports._printStdoutError = function(msg) {

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Nov 15, 2015

Contributor

Why not directly do this in printStdoutError itself?

@JungMinu

This comment has been minimized.

Copy link
Member Author

commented Nov 15, 2015

@thefourtheye Thanks, updated :)

@thefourtheye

View changes

lib/events.js Outdated
@@ -232,7 +233,7 @@ EventEmitter.prototype.addListener = function addListener(type, listener) {
m = $getMaxListeners(this);
if (m && m > 0 && existing.length > m) {
existing.warned = true;
console.error('(node) warning: possible EventEmitter memory ' +
iutil.printStdoutError('warning: possible EventEmitter memory ' +
'leak detected. %d %s listeners added. ' +

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Nov 15, 2015

Contributor

The string should align with the previous line's string.

@thefourtheye

View changes

lib/_debugger.js Outdated
@@ -31,7 +32,7 @@ exports.start = function(argv, stdin, stdout) {
stdin.resume();

process.on('uncaughtException', function(e) {
console.error("There was an internal error in Node's debugger. " +
iutil.printStdoutError("There was an internal error in Node's debugger. " +
'Please report this bug.');

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Nov 15, 2015

Contributor

The string should align with the previous line's string.

@thefourtheye

View changes

lib/internal/util.js Outdated
@@ -14,6 +14,14 @@ exports.printDeprecationMessage = function(msg, warned) {
return exports._printDeprecationMessage(`${prefix}${msg}`, warned);
};

exports.printStdoutError = function(msg) {

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Nov 15, 2015

Contributor

We can keep the names short, may be error and trace itself?

This comment has been minimized.

Copy link
@evanlucas

evanlucas Nov 15, 2015

Member

I agree, especially since this prints to stderr and not stdout

This comment has been minimized.

Copy link
@jasnell

jasnell Nov 16, 2015

Member

+1 ... everything else looks good. shorter names here would be better.

@silverwind

View changes

lib/_debugger.js Outdated
@@ -1,5 +1,6 @@
'use strict';

const iutil = require('internal/util');

This comment has been minimized.

Copy link
@silverwind

silverwind Nov 15, 2015

Contributor

We usually call it internalUtil.

@silverwind

View changes

lib/_tls_common.js Outdated
@@ -1,5 +1,7 @@
'use strict';

const iutil = require('internal/util');
const util = require('util');

This comment has been minimized.

Copy link
@silverwind

silverwind Nov 15, 2015

Contributor

util isn't needed here, I think.

@jwueller

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2015

@bnoordhuis (node) [pid] would be grepable. (node:pid) is as well, but not as easily. I find that to be very useful.

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Nov 15, 2015

I would think that (node:pid) is easier to grep for, or at least more precisely. With a naive grep '(node) [1234]', the [1234] gets interpreted as (1|2|3|4).

@jwueller

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2015

Yes, the [] are probably a poor choice, but if I am not mistaken, there is more than once place in node where the (node) prefix is used. Making that consistent sounds like the right thing to do. Additionally providing the PID is a good thing, but it feels like that should not interfere with the existing pattern.

@jwueller

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2015

Oh wait, are we changing the prefix for everything? In that case, I am good with (node:pid) everywhere! Sorry about the confusion.

@JungMinu

This comment has been minimized.

Copy link
Member Author

commented Nov 16, 2015

@silverwind @bnoordhuis @thefourtheye Thanks, updated 😄

@thefourtheye

View changes

lib/_debugger.js Outdated
@@ -31,8 +32,8 @@ exports.start = function(argv, stdin, stdout) {
stdin.resume();

process.on('uncaughtException', function(e) {
console.error("There was an internal error in Node's debugger. " +
'Please report this bug.');
internalUtil.error("There was an internal error in Node's debugger. " +

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Nov 16, 2015

Contributor

We normally use ' for string literals, but in this case it is okay, I guess.

This comment has been minimized.

Copy link
@JungMinu

JungMinu Nov 16, 2015

Author Member

@thefourtheye Because of ' in Node's , we can't use ' for string literals in this case.

This comment has been minimized.

Copy link
@Fishrock123

Fishrock123 Nov 16, 2015

Member

I'd prefer we just escape the apostrophe like so:

'There was an internal error in Node\'s debugger. '

This comment has been minimized.

Copy link
@Fishrock123

Fishrock123 Nov 16, 2015

Member

Or alternatively, just use a template string.

This comment has been minimized.

Copy link
@JungMinu

JungMinu Nov 16, 2015

Author Member

@Fishrock123 Thanks :)

@thefourtheye

View changes

lib/_debugger.js Outdated
console.error("There was an internal error in Node's debugger. " +
'Please report this bug.');
internalUtil.error("There was an internal error in Node's debugger. " +
'Please report this bug.');

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Nov 16, 2015

Contributor

This should align with the string literal in the previous line.

@thefourtheye

View changes

lib/_debugger.js Outdated
@@ -1710,7 +1711,7 @@ Interface.prototype.trySpawn = function(cb) {
function connectError() {
// If it's failed to connect 10 times then print failed message
if (connectionAttempts >= 10) {
console.error(' failed, please retry');
internalUtil.error(' failed, please retry');

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Nov 16, 2015

Contributor

I think this error message could be more descriptive. If you like to do it, you can do it now.

@thefourtheye

View changes

lib/events.js Outdated
'Use emitter.setMaxListeners() to increase limit.',
existing.length, type);
internalUtil.error('warning: possible EventEmitter memory ' +
'leak detected. %d %s listeners added. ' +

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Nov 16, 2015

Contributor

This string literal should align with the string literal in the previous line.

@thefourtheye

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2015

Overall change LGTM

@JungMinu

This comment has been minimized.

Copy link
Member Author

commented Nov 16, 2015

@thefourtheye updated :)

@evanlucas

This comment has been minimized.

Copy link
Member

commented Nov 16, 2015

Can you run make test on this? This change seems to make all tests fail

internal/util.js:3
const prefix = `(node:${process.pid}) `;
                        ^

ReferenceError: process is not defined
@thefourtheye

View changes

src/cares_wrap.cc Outdated
@@ -205,7 +205,7 @@ static void ares_sockstate_cb(void* data,
} else {
/* read == 0 and write == 0 this is c-ares's way of notifying us that */
/* the socket is now closed. We must free the data associated with */
/* socket. */
/* the socket. */

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Nov 21, 2015

Contributor

Is this really necessary as part of this PR?

@thefourtheye

View changes

src/cares_wrap.cc Outdated
@@ -631,7 +631,7 @@ class QueryTxtWrap: public QueryWrap {
}
txt_chunk->Set(j++, txt);
}
// Push last chunk if it isn't empty
// Push the last chunk if it isn't empty

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Nov 21, 2015

Contributor

Is this really necessary as part of this PR?

@thefourtheye

View changes

src/node_file.cc Outdated
return TYPE_ERROR("src path required");
if (len < 2)

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Nov 21, 2015

Contributor

Why changes in this file are necessary as part of this PR?

@JungMinu

This comment has been minimized.

Copy link
Member Author

commented Nov 21, 2015

@jasnell @thefourtheye finished and rebased, Thanks :)

@@ -1737,7 +1738,7 @@ Interface.prototype.trySpawn = function(cb) {
function connectError() {
// If it's failed to connect 10 times then print failed message
if (connectionAttempts >= 10) {
console.error(' failed, please retry');
internalUtil.error(' failed to connect, please retry');

This comment has been minimized.

Copy link
@silverwind

silverwind Nov 22, 2015

Contributor

Not sure about the context of this error, but the message change seems unnecessary.

This comment has been minimized.

Copy link
@JungMinu

JungMinu Nov 23, 2015

Author Member

@silverwind Editing this error message is suggested by @thefourtheye

I think this error message could be more descriptive. If you like to do it, you can do it now.

This comment has been minimized.

Copy link
@silverwind

silverwind Nov 23, 2015

Contributor

Ah, alright. Guess it's ok then :)

@silverwind

This comment has been minimized.

Copy link
Contributor

commented Nov 23, 2015

LGTM. What happened to the C++ ambitions?

@silverwind

This comment has been minimized.

Copy link
Contributor

commented Nov 23, 2015

@JungMinu

This comment has been minimized.

Copy link
Member Author

commented Nov 23, 2015

@silverwind I found that the problem was not from C++,but there was a mistake in test script for new functions. I fixed it in my previous PR :)
Is there any error from c++ that I missed?

@silverwind

This comment has been minimized.

Copy link
Contributor

commented Nov 23, 2015

Searching for printf in src reveals quite a few cases, like here. I suppose C++ could be done in another PR, and these prints should be handled on a per-case basis. I suggest not adding the prefix when the process terminates after the error.

@JungMinu

This comment has been minimized.

Copy link
Member Author

commented Dec 3, 2015

I've added more C++ files in src, please review again :)

@JungMinu JungMinu changed the title lib: Ensure '(node:pid)' prefix for stdout logging lib,src: Ensure '(node:pid)' prefix for stdout logging Dec 3, 2015

@silverwind

View changes

test/parallel/test-sync-io-option.js Outdated
@@ -20,7 +20,7 @@ if (process.argv[2] === 'child') {
execFile(process.execPath, args, function(err, stdout, stderr) {
assert.equal(err, null);
assert.equal(stdout, '');
if (/^WARNING[\s\S]*fs\.readFileSync/.test(stderr))
if (/^[\s\S]*fs\.readFileSync/.test(stderr))

This comment has been minimized.

Copy link
@silverwind

silverwind Dec 3, 2015

Contributor

I'd suggest you just remove the ^ here, so the regex is /WARNING[\s\S]*fs\.readFileSync/.

@JungMinu

This comment has been minimized.

Copy link
Member Author

commented Dec 3, 2015

@silverwind Thanks, fixed :)

@silverwind

View changes

src/node_crypto.cc Outdated
@@ -813,7 +813,7 @@ void SecureContext::SetDHParam(const FunctionCallbackInfo<Value>& args) {
return env->ThrowError("DH parameter is less than 1024 bits");
} else if (size < 2048) {
args.GetReturnValue().Set(FIXED_ONE_BYTE_STRING(
env->isolate(), "WARNING: DH parameter is less than 2048 bits"));
env->isolate(), "(node) WARNING: DH parameter is less than 2048 bits"));

This comment has been minimized.

Copy link
@silverwind

silverwind Dec 3, 2015

Contributor

Can we get a PID here too?

This comment has been minimized.

Copy link
@JungMinu

JungMinu Dec 3, 2015

Author Member

@silverwind this gets logged in here, which is already handled in js code :)

@JungMinu

This comment has been minimized.

Copy link
Member Author

commented Dec 3, 2015

updated and finished :)

@silverwind

View changes

src/node_crypto.cc Outdated
env->isolate(), "WARNING: DH parameter is less than 2048 bits"));
}
env->isolate(), "WARNING: DH parameter is less than 2048 bits"));
}

This comment has been minimized.

Copy link
@silverwind

silverwind Dec 3, 2015

Contributor

Remove the added spaces here.

This comment has been minimized.

Copy link
@JungMinu

JungMinu Dec 3, 2015

Author Member

Yep, I am doing that :)

This comment has been minimized.

Copy link
@silverwind

silverwind Dec 3, 2015

Contributor

Btw, git reset origin/master src/node_crypto.cc could help in resetting just that file :)

@JungMinu

This comment has been minimized.

Copy link
Member Author

commented Dec 3, 2015

@silverwind fixed, thanks :)

lib,src: Ensure '(node:pid)' prefix for stdout logging
add '(node:pid)' prefix message for stdout logging
@silverwind

This comment has been minimized.

Copy link
Contributor

commented Dec 3, 2015

@silverwind

This comment has been minimized.

Copy link
Contributor

commented Dec 3, 2015

Failures are unrelated, LGTM

@jasnell

This comment has been minimized.

Copy link
Member

commented Dec 3, 2015

LGTM

jasnell added a commit that referenced this pull request Dec 3, 2015

lib,src: ensure '(node:pid)' prefix for stdout logging
Add '(node:pid)' prefix message for stdout logging

PR-URL: #3833
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
@jasnell

This comment has been minimized.

Copy link
Member

commented Dec 3, 2015

Landed in 94b9948

@jasnell jasnell closed this Dec 3, 2015

@rvagg

This comment has been minimized.

Copy link
Member

commented Dec 5, 2015

marking as semver-major so it doesn't get caught up in v5.x, I think that's appropriate, please correct me if I'm wrong.

@rvagg rvagg added the semver-major label Dec 5, 2015

@silverwind

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2015

@rvagg well, it could certainly break things that parse our stdout, so I guess it's fine to conservatibely label it as major.

@jasnell jasnell referenced this pull request Mar 17, 2016

Closed

Planning for v6 #5766

scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016

lib,src: ensure '(node:pid)' prefix for stdout logging
Add '(node:pid)' prefix message for stdout logging

PR-URL: nodejs#3833
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.