Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Ensuring private Buffer require in lib/* files #8603

Closed
wants to merge 1 commit into from

Conversation

odeke-em
Copy link

This addresses issue #8588 in which Buffer was being used globally in lib/* files. Any redefinition of Buffer when using a repl would crash. Credits to @indutny and @vkurchatkin for providing the remedy.

@indutny
Copy link
Member

indutny commented Oct 23, 2014

cc @trevnorris

@odeke-em : could you please take a look at CONTRIBUTING.md to see how your commit message should be improved to be accepted? ;)

@odeke-em
Copy link
Author

Thanks @indutny , I have taken a look at it and made an amend to the commit message. Please help me check if it is appropriate.

@trevnorris
Copy link

Commit message first line should not be longer than 50 characters.

Other than that LGTM.

@odeke-em
Copy link
Author

Thanks for the patience.

@trevnorris
Copy link

Thanks. Now, I just barely ran the tests. Things are falling over quite badly. Issue is there are places where require('buffer') isn't yet available. In src/node.js we get around this by using NativeModule but that doesn't exist elsewhere. Again, we're relying on a global (require()) and assuming all modules will be available all the time. That just isn't the case.

@odeke-em
Copy link
Author

Hey @trevnorris, I guess I bit on more than I could chew: could I kindly ask if someone with more knowledge of the internals [ like a core developer ] look at this since we'd probably want this done well? Instead of me holding you and @indutny back.

@trevnorris
Copy link

@odeke-em No worries at all. Honestly I can't think of a good solution ATM. I'll leave this open and if you want to keep experimenting with it feel free.

@vkurchatkin
Copy link

Looks like a simple case of circular dependencies. Try this:

diff --git a/lib/assert.js b/lib/assert.js
index 0ab3f49..5bd30ea 100644
--- a/lib/assert.js
+++ b/lib/assert.js
@@ -24,7 +24,7 @@

 // UTILITY
 var util = require('util');
-var Buffer = require('buffer').Buffer;
+var b = require('buffer');
 var pSlice = Array.prototype.slice;

 // 1. The assert module provides functions that throw
@@ -145,7 +145,7 @@ function _deepEqual(actual, expected) {
   if (actual === expected) {
     return true;

-  } else if (Buffer.isBuffer(actual) && Buffer.isBuffer(expected)) {
+  } else if (b.Buffer.isBuffer(actual) && b.Buffer.isBuffer(expected)) {
     if (actual.length != expected.length) return false;

     for (var i = 0; i < actual.length; i++) {

Fixes usage of global object 'Buffer' in lib/* files by ensuring
that each file does an explicit require('buffer').Buffer.
Previously, when running a repl, due to usage of global 'Buffer',
any redefinition of Buffer would cause a crash eg var Buffer = {}.
@odeke-em
Copy link
Author

@trevnorris For sure, will do.

@odeke-em
Copy link
Author

@vkurchatkin Thanks! Your solution cuts the deal, tests related to those modules on mine pass. Only thing tripped out on mine are errors related to EMFILE [too many files open], but I don't think that's directly related to the issue at hand.

@odeke-em
Copy link
Author

@trevnorris ping!

trevnorris pushed a commit that referenced this pull request Oct 25, 2014
Fixes usage of global object 'Buffer' in lib/* files by ensuring that
each file does an explicit require('buffer').Buffer.  Previously, when
running a repl, due to usage of global 'Buffer', any redefinition of
Buffer would cause a crash eg var Buffer = {}.

Fixes: #8588
PR-URL: #8603
Reviewed-by: Trevor Norris <trev.norris@gmail.com>
@trevnorris
Copy link

Thanks. Merged into 523929c.

@trevnorris trevnorris closed this Oct 25, 2014
@odeke-em
Copy link
Author

Thanks for merging this in.

mscdex pushed a commit to mscdex/node that referenced this pull request Dec 25, 2014
Fixes usage of global object 'Buffer' in lib/* files by ensuring that
each file does an explicit require('buffer').Buffer.  Previously, when
running a repl, due to usage of global 'Buffer', any redefinition of
Buffer would cause a crash eg var Buffer = {}.

Fixes: nodejs#8588
PR-URL: nodejs#8603
Reviewed-by: Trevor Norris <trev.norris@gmail.com>
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request May 22, 2015
Fixes usage of global object 'Buffer' in lib/* files by ensuring that
each file does an explicit require('buffer').Buffer.  Previously, when
running a repl, due to usage of global 'Buffer', any redefinition of
Buffer would cause a crash eg var Buffer = {}.

Fixes: nodejs/node-v0.x-archive#8588
PR-URL: nodejs/node-v0.x-archive#8603
Reviewed-by: Trevor Norris <trev.norris@gmail.com>

Conflicts:
	lib/_stream_readable.js
	lib/_stream_writable.js
	lib/assert.js
	lib/dgram.js
	lib/fs.js
	lib/http.js
	lib/net.js
	lib/readline.js
	lib/tls.js
	lib/zlib.js
silverwind added a commit to nodejs/node that referenced this pull request Jun 11, 2015
Port of nodejs/node-v0.x-archive#8603

The race condition present in the original PR didn't occur, so no
workaround was needed.

PR-URL: #1794
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants