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

repl: don't crash if self is not in replMap #7718

Merged
merged 1 commit into from
Jul 26, 2016

Conversation

evanlucas
Copy link
Contributor

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

repl

Description of change

Fixes: #7716

This fixes an undefined referenced. I am still trying to figure out why the undefined reference is occurring though, so marking as WIP.

@evanlucas
Copy link
Contributor Author

/cc @Trott since he seems to be familiar with the code (b354be7)

@Trott
Copy link
Member

Trott commented Jul 13, 2016

Paging REPL folks: @addaleax @Fishrock123

@Trott Trott added the wip Issues and PRs that are still a work in progress. label Jul 13, 2016
@Fishrock123
Copy link
Contributor

Sorry, no idea what's up here.

@Trott
Copy link
Member

Trott commented Jul 15, 2016

@evanlucas Alternatively, this will fix it too:

diff --git a/lib/repl.js b/lib/repl.js
index ed89655..ab61624 100644
--- a/lib/repl.js
+++ b/lib/repl.js
@@ -487,6 +487,7 @@ function REPLServer(prompt,
     }

     debug('eval %j', evalCmd);
+    replMap.set(self, self);
     self.eval(evalCmd, self.context, 'repl', finish);

     function finish(e, ret) {

@evanlucas
Copy link
Contributor Author

@Trott that does work for me as well. Want to submit a PR for it? Or should I just change it here?

@addaleax
Copy link
Member

This patch seems to be the Right Way™ to stop the undefined reference from occurring in the first place:

diff --git a/lib/repl.js b/lib/repl.js
index ed896552656c..82f532f42e5f 100644
--- a/lib/repl.js
+++ b/lib/repl.js
@@ -727,12 +727,12 @@ function complete(line, callback) {
     });
     var flat = new ArrayStream();         // make a new "input" stream
     var magic = new REPLServer('', flat); // make a nested REPL
+    replMap.set(magic, replMap.get(this));
     magic.context = magic.createContext();
     flat.run(tmp);                        // eval the flattened code
     // all this is only profitable if the nested REPL
     // does not have a bufferedCommand
     if (!magic.bufferedCommand) {
-      replMap.set(magic, replMap.get(this));
       return magic.complete(line, callback);
     }
   }

(magic.bufferedCommand is empty in the scenario from #7716 because TAB is pressed at the beginning of a line, I think.)

I’m inclined to say that some larger refactoring of the entire REPL completion implementation might be a good idea in the long run.

@Trott
Copy link
Member

Trott commented Jul 15, 2016

I'm not sure which is better--I can think of hypothetical pros and cons for both, although they might not hold up to closer scrutiny of the code. And it's possible that there's a third fix lurking in there. (I have an idea half-formed.)

If this isn't super-critical to fix RIGHT AWAY, I think what I'd like to do is get the code coverage up a bit by adding other tests first, just to get a few more wonky edge cases in the mix. There's about 54 statements in repl.js that aren't covered by tests right now. https://node-core-coverage.addaleax.net/coverage-4220c24b17955f7f/lib/repl.js.html

Then, once the tests are in place, try out both fixes, and maybe a third option if I can put it together, and see how they all do. Maybe try to break one or the other to see if we can.

Alternatively, if you want to get this fixed ASAP, by all means, add a test and land either fix.

@Trott
Copy link
Member

Trott commented Jul 15, 2016

Oh, I should add that there's an excellent chance what's going on is that I or someone else failed to account for something or other about domain behavior. It's been a while since I've had to be aware of that stuff...

@evanlucas
Copy link
Contributor Author

What's really weird is that the test from test/parallel/test-repl-tab-complete-crash.js should not be passing. The exit event is never fired on process. The completion handler should take a callback...still digging into this though

@addaleax
Copy link
Member

@Trott That comment was written before I posted mine, right?

The exit event is never fired on process.

Huh. I’d guess there’s a good chance I broke that test in 3ee68f7

@Trott
Copy link
Member

Trott commented Jul 15, 2016

@addaleax wrote:

@Trott That comment was written before I posted mine, right?

Indeed! You have the better grasp on the totality of issues here.

@princejwesley
Copy link
Contributor

@Trott REPLServer.prototype.convertToContext is unused function.

@addaleax
Copy link
Member

@evanlucas Do you want to use my patch for this PR?

The reason test/parallel/test-repl-tab-complete-crash.js passes is that the process.on('exit', …) line is never reached, the completion fails silently without any error…

@evanlucas
Copy link
Contributor Author

evanlucas commented Jul 19, 2016

@addaleax yea, I've been working on fixing that test as well. That test should still call the event handler, IMO. I have traced the reason back to the domain exiting. So, not really sure if that is another bug in itself or not...

I'll be able to update tonight/tomorrow hopefully

@evanlucas evanlucas removed the wip Issues and PRs that are still a work in progress. label Jul 22, 2016
@evanlucas
Copy link
Contributor Author

Ok, updated. PTAL

@addaleax
Copy link
Member

LGTM with CI (which I would start if loading Jenkins didn’t take, like, ages rn)

@evanlucas
Copy link
Contributor Author

evanlucas commented Jul 23, 2016

@evanlucas evanlucas changed the title [WIP] repl: don't crash if self is not in replMap repl: don't crash if self is not in replMap Jul 23, 2016
Fixes: nodejs#7716
PR-URL: nodejs#7718
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@evanlucas
Copy link
Contributor Author

Landed in 392c70a. Thanks!

@evanlucas evanlucas merged commit 392c70a into nodejs:master Jul 26, 2016
evanlucas added a commit that referenced this pull request Aug 2, 2016
Fixes: #7716
PR-URL: #7718
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@cjihrig cjihrig mentioned this pull request Aug 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[repl] Tab completion crashes when invoked on in multiline mode after line with error
7 participants