fs: name anonymous functions by fhalde · Pull Request #9252

fs: name anonymous functions by fhalde · Pull Request #9252 · nodejs/node

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

Description of change

@7coder7 You may have to fix the commit. It says that it was made on May 25th.

@7coder7 Yes. It looks fine now. But, it looks like there are test failures.

@thefourtheye had to change the test cases test/parallel/test-sync-io-option.js because the regex condition was failing while searching for the module function-name in the stacktrace

		assert.equal(err, null);
		assert.equal(stdout, '');
		if (/WARNING[\s\S]*fs\.readFileSync/.test(stderr))
		if (/WARNING[\s\S]*readFileSync/.test(stderr))

Choose a reason for hiding this comment

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

Naming anonymous functions produces a stacktrace with just the function name now. For example, earlier it used to be fs.readFileSync and now it has become just readFileSync in the stacktrace.

Is that okay @nodejs/collaborators ?

Choose a reason for hiding this comment

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

Maybe it's possible to modify Environment::PrintSyncTrace to get it back ?
It's not affecting the stack traces generated by V8 when an error is thrown.

Choose a reason for hiding this comment

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

I'm fine with this but @targos' suggestion is good also.

Choose a reason for hiding this comment

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

@targos I think v8 doesn't provide a way to get the actually bound object name. Is there any APIs which we can leverage? Also, PrintSyncTrace has only references to the Stack Frames. They just give only the debug function names.

Choose a reason for hiding this comment

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

@thefourtheye yeah it seems that the API doesn't give us access to more information. I'm fine with the change too.

Choose a reason for hiding this comment

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

Isn't the purpose of adding the names to improve stack trace? Maybe we ought to only add names in places where the stack trace can't determine the name? Like, anonymous callback functions, but not functions that are assigned to a property on a variable because the name is inferred from the assignment?

Trott left a comment •

Loading

Choose a reason for hiding this comment

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

Since the purpose of naming anonymous functions is to improve stack traces, and this seems to have the opposite effect, I'm actually -1 on this and similar changes, I think. I'm OK with this sort of thing in cases where the name cannot be inferred, but the name can be inferred in all the instances in this change, and the name that V8 infers is better than the name we provide in this change.

Choose a reason for hiding this comment

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

I too have concerns about this. If fs.func turns to func, it is clearly a regression in my eyes. This can be particularly problematic in net and decendants where many functions are named the same. V8's function name inferring is bound to get better over time, too.

See #8913 (comment) for a suggestion.

Choose a reason for hiding this comment

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

I am against this change, as all the named functions will have names inferred by v8.

We should name only the closures.

		ReadStream.prototype._read = function(n) {
		ReadStream.prototype._read = function _read(n) {
		if (typeof this.fd !== 'number')
		return this.once('open', function() {

Checklist

Affected core subsystem(s)

Description of change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott left a comment • Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott left a comment •

Loading