Do not cache modules that throw exceptions #710

Closed
wants to merge 7 commits into
from

Projects

None yet

8 participants

@wdacgrs

Should be strlen.

Node.js Foundation member

should not be strlen, because that would provoke all kinds of bad when the buffer contains no \0 chars.

I prompt this because the source failed to compile... maybe missing header?

no strnlen on OSX :)

ry and others added some commits Feb 15, 2011
@ry ry Revert "buffer.toString() shouldn't include null values"
This reverts commit 909a5b3.

Will fix inside V8's String::New instead.
0474ce6
@ry ry Merge branch 'v0.4' 9851574
@isaacs isaacs Closes GH-695 Add 'hex' encoding to Buffer 0aa1a8a
@ry ry Merge branch 'v0.4'
Conflicts:
	src/node_version.h
7dad30a
@felixge felixge Do not cache modules that throw exceptions
If a module throws an exception on load, it should not be cached.
This patch shows the problem in a test case and also fixes it.

See: https://groups.google.com/forum/#!topic/nodejs-dev/1cIrvJcADbY
89d752f
@isaacs

The test is good. However, I think this needs to be done in a way that doesn't wrap all modules in a try/catch. How were we doing this before?

The try...catch should only effect the first execution of the modules code, later callbacks should not be affected by it. I don't see a problem, do you?

Anyway, a better way would be to not cache the module until it has executed, but this breaks cyclic self-reference dependencies afaik (the test for it failed). Got an idea?

I'll benchmark it to make sure. Last I checked, even defining the function inside the try/catch made it slower. If it's only an initial hit, then this approach is ideal.

I was thinking we could do something where we have a provisional cache, and then move modules into the "real" cache after they're run. But there's no easy way to know whether it threw, or just didn't finish yet. It would require a bit of clever thinking to come up with an unclever way to make that approach work.

Ok, ping me with the benchmark results.

Nevermind, I'm either thinking of something else, or smoking the crack again. It's fine. Just statistical noise in the benchmarks I ran.

Alright, lets hoep ryan merges this soon : )

@isaacs

+1

@felixge

Do not cache modules that throw exceptions

If a module throws an exception on load, it should not be cached.
This patch shows the problem in a test case and also fixes it.

See: https://groups.google.com/forum/#!topic/nodejs-dev/1cIrvJcADbY

Closed by 66601f1
Closed by 66601f1

@coolaj86 coolaj86 pushed a commit that referenced this pull request Apr 15, 2011
@felixge felixge Do not cache modules that throw exceptions
If a module throws an exception on load, it should not be cached.
This patch shows the problem in a test case and also fixes it.

See: https://groups.google.com/forum/#!topic/nodejs-dev/1cIrvJcADbY

Closes GH-707
Closes GH-710
66601f1
@bkw

any chance of also getting this back into v0.4 ?

Node.js Foundation member

Please open an issue if you feel strongly about it, it's unfortunately not as trivial as simply applying the patch. Some of the internals have changed so it doesn't do quite the right thing without some work.

in that case, I'll let it rest and use a wrapper instead. Thanks for responding so quickly.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment