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

docs: why we use try-finally over try-catch #18711

Closed
wants to merge 3 commits into from
Closed

docs: why we use try-finally over try-catch #18711

wants to merge 3 commits into from

Conversation

oliviertassinari
Copy link

@oliviertassinari oliviertassinari commented Feb 11, 2018

Unless I'm missing something, we can simplify the logic.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

Unless I'm missing something, we can simplify the logic.
@nodejs-github-bot nodejs-github-bot added the module Issues and PRs related to the module subsystem. label Feb 11, 2018
@vsemozhetbyt
Copy link
Contributor

@targos
Copy link
Member

targos commented Feb 11, 2018

The difference is that the error was not caught before. But if you rethrow it after updating the cache I agree it would simplify the logic

delete Module._cache[filename];
}
} catch (err) {
delete Module._cache[filename];
Copy link
Member

Choose a reason for hiding this comment

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

This silently suppresses the error. You need to throw err.

tniessen
tniessen previously approved these changes Feb 11, 2018
lib/module.js Outdated
}
} catch (err) {
delete Module._cache[filename];
throw err
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Missing semicolon.

targos
targos previously approved these changes Feb 11, 2018
@vsemozhetbyt
Copy link
Contributor

@tniessen tniessen dismissed their stale review February 11, 2018 13:46

Meant to comment.

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

I am against landing this. The reason to the current implementation is that the error does not have to be rethrown. Rethrowing is bad in the way that it changes the origin of the error to module.js.

I guess adding a comment would be good though.

@oliviertassinari
Copy link
Author

Rethrowing is bad in the way that it changes the origin of the error to module.js.

@BridgeAR I didn't know that. I thought the origin of the error was defined by where the Error object is instantiated. Can it also have some performance implication?

@BridgeAR
Copy link
Member

So the stack frames stay the same but the output where the error was created will be different.

It should not have any performance implications.

@targos targos dismissed their stale review February 11, 2018 18:38

breaks error output

@targos
Copy link
Member

targos commented Feb 11, 2018

+1 to adding a comment

@TimothyGu
Copy link
Member

So the stack frames stay the same but the output where the error was created will be different.

Hmm, I can think of a case where this might not be true. I will try to confirm it later today.

@Fishrock123 Fishrock123 added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 11, 2018
@Fishrock123
Copy link
Contributor

This changes what is in cache if the error is caught from what would have previously been. Is this something we should pay attention to?

@benjamingr
Copy link
Member

Sorry, I don't understand why this is semver-major, the code looks equivalent (after the rethrow being added) - can anyone explain?

@apapirovski
Copy link
Member

Given that this changes the error output and requires a number of tests to be updated, should this even be landing? Using catch and re-throwing doesn't seem better to me than using finally with an extra variable check...

Not quite certain how I feel about this change.

@benjamingr
Copy link
Member

@apapirovski why/where does it change the error?

@apapirovski
Copy link
Member

@benjamingr It changes the origin, as mentioned above, hence all the failing tests.

For example, instead of

assert.js:*
  throw new AssertionError(obj);

we get

module.js:520
    throw err;

@benjamingr
Copy link
Member

@apapirovski that's surprising to me, I thought stack traces are created when an error is created and not when it is thrown - so rethrowing an error shouldn't change the stack trace. Thanks.

@BridgeAR
Copy link
Member

@benjamingr the stack trace is still identical. Just the origin changes.

@benjamingr
Copy link
Member

benjamingr commented Feb 12, 2018

Thanks, so it changes the file name and not the trace - got it.


@apapirovski are you sure?

var e;
function a() {
  function b() {
    throw new Error();
  }
  b();
}
try {
  a();
} catch (err) {
  e = err;
}
function c() {
  function d() {
    throw e;
  }
  d();
}
try {
  c();
} catch (err) {
  console.log(err);
}

Logs:

Error
    at b (<anonymous>:4:11)
    at a (<anonymous>:6:3)
    at <anonymous>:9:3

@apapirovski
Copy link
Member

@benjamingr Yep, certain. The console.log only logs the stack trace, not the origin of the error which is created on throw. You can even try in Chrome where you'll need to expand the error to see the origin of the error, as opposed to the stack trace.

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Given this breaks tests and doesn't make the code much more readable - I'm -1 on this (since it'll require additional changes).

Thanks a lot for your contribution - and if you'd like help in finding things to work on feel free to ping me personally or ask.

@BridgeAR
Copy link
Member

@oliviertassinari if you would be willing to change your code to a comment that highlights why the current implementation is necessary, I am certain it will land :-)

@oliviertassinari
Copy link
Author

if you would be willing to change your code to a comment that highlights why the current implementation is necessary

@BridgeAR Great. I'm on it. I wasn't expected as much activity. Thanks for the reviews.

@oliviertassinari oliviertassinari changed the title Use catch over finally docs: use catch over finally Feb 16, 2018
@oliviertassinari oliviertassinari changed the title docs: use catch over finally docs: why we use try-finally over try-catch Feb 16, 2018
@oliviertassinari
Copy link
Author

oliviertassinari commented Feb 16, 2018

I have blamed the source and found #5172 as the origin of the change. It might performance related too. I'm abandoning this pull-request, cloning node is quick slow on my end:

Receiving objects: 47% (178420/376085), 203.59 MiB | 563.00 KiB/s

Thanks for the feedback guys!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module Issues and PRs related to the module subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.