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: limit line processing to one at a time #39392

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

ejose19
Copy link
Contributor

@ejose19 ejose19 commented Jul 14, 2021

Fixes: #39387

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. repl Issues and PRs related to the REPL subsystem. labels Jul 14, 2021
@ejose19
Copy link
Contributor Author

ejose19 commented Jul 15, 2021

@guybedford Feel free to take a look at this

lib/repl.js Outdated Show resolved Hide resolved
Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Can you please clarify if any of the changes here are major / breaking changes?

//cc @nodejs/repl for more reviewers.

test/parallel/test-repl-autolibs.js Outdated Show resolved Hide resolved
test/parallel/test-repl-empty.js Show resolved Hide resolved
@ejose19
Copy link
Contributor Author

ejose19 commented Jul 15, 2021

I'd like to add the output of debug to my test assertions, so I confirm the queue flow is happening as expected, but didn't see any other repl test that is asserting debug output (and using NODE_DEBUG=repl before calling the tests makes the output end up in my terminal instead of repl output stream), any ideas how I can do it?

@guybedford guybedford added dont-land-on-v12.x semver-major PRs that contain breaking changes and should be released in the next major version. labels Jul 15, 2021
@guybedford
Copy link
Contributor

@ejose19 I'm not aware of any debug stream testing, but there might be some examples in the existing tests directory to be found. Alternatively setting up something custom could be worthwhile unless someone else can clarify further here.

@ejose19
Copy link
Contributor Author

ejose19 commented Jul 15, 2021

@guybedford I had to replace the original debuglog function before importing repl in order to access debug data. That should be enough for this test.

@guybedford
Copy link
Contributor

@ejose19 if we're doing a major change here anyway, should we also support eval returning a Promise? Or is the callback definitely necessary to get the correct timings?

@ejose19
Copy link
Contributor Author

ejose19 commented Jul 16, 2021

@guybedford That would require a major overhaul in the tests, since most of them expect the output right after writing the input, which is the case for sync code. But if eval is async, code execution (and output) would be done in the next iteration.

So even if using async eval would be ideal, it's outside the scope of this PR due the large amount of changes.

@guybedford
Copy link
Contributor

@ejose19 what I mean is branching the API on sync / async handling with:

eval: () => Promise | undefined

where the undefined case is treated as sync, and the promise case is treated as the async situation, for example rather than enforcing the callback. That's not the exact approach but along those lines I mean.

@ejose19
Copy link
Contributor Author

ejose19 commented Jul 16, 2021

@guybedford you mean something like this?

const evalResult = self.eval(evalCmd, self.context, getREPLResourceName(), finish);

if (evalResult !== undefined) {
 if (!(evalResult instanceof Promise)) {
   self._domain.emit('error', 'eval must return "undefined" or a "Promise"');
   return;
  }

  (async () => {
    const { error, result } = await evalResult;
    finish(error, result);
  })();
}

@guybedford
Copy link
Contributor

Yes that seems like the sort of approach if you agree it could make sense. Would this also allow this PR to be done in a backwards compatible way perhaps? Perhaps we should consider being more lenient on the return value to be safe in that case? Either way wrt the validation could work for me though.

@ejose19
Copy link
Contributor Author

ejose19 commented Jul 16, 2021

I don't think it can be made backward compatible, we can't assume undefined = "sync code", as it's not even the case for node own defaultEval (notice how the promise is not returned):

node/lib/repl.js

Lines 606 to 623 in 44e3822

(async () => {
try {
const result = await promise;
finishExecution(null, result);
} catch (err) {
if (err && process.domain) {
debug('not recoverable, send to domain');
process.domain.emit('error', err);
process.domain.exit();
return;
}
finishExecution(err);
} finally {
// Remove prioritized SIGINT listener if it was not called.
prioritizedSigintQueue.delete(sigintListener);
unpause();
}
})();

If other projects based their eval implementation following node defaultEval, then undefined can also mean promise handling.

Ideally, self.on('line') should be refactored into for await (const cmd of self), so line processing is guaranteed to be one at a time without needing a queue, and eval should be made a Promise resolving to { err?: any, result?: any }, to avoid the need of a callback completely and making the implementation more direct. But that comes with all the tests refactor and the "breaking" change of writing to stdin will no longer yield a response in the current tick.

@guybedford
Copy link
Contributor

@ejose19 I guess I'm still not clear on how the callback can be optional currently. But if the current eval doesn't return a value, then allowing the current eval to return a promise would be a backwards-compatible extension. Then allowing that promise to be a completion based callback that fits the requirements you needed for the async callback could also be a way to possible shoehorn this feature in a backwards compatible way (separate to the explicit callback mechanism). Again I'm not super clear on the details of this code, so fill me in where I'm wrong :)

@ejose19
Copy link
Contributor Author

ejose19 commented Jul 16, 2021

@guybedford you mean something like this?

const evalResult = self.eval(evalCmd, self.context, getREPLResourceName(), finish);

if (evalResult !== undefined) {
 if (!(evalResult instanceof Promise)) {
   self._domain.emit('error', 'eval must return "undefined" or a "Promise"');
   return;
  }

  (async () => {
    const { error, result } = await evalResult;
    finish(error, result);
  })();
}

@guybedford If something like this is added to the PR, then eval must either:

  • Return a Promise that returns { err?: any, result?: any }
  • Execute callback

For any code that is not returning a Promise and neither was executing the callback, it WILL be a breaking change since new lines won't be allowed to be processed (you noticed that it was necessary to execute the callback on tests, and probably some consumers have a similar implementation).

So what I mean is, while we can improve the "handling" of eval supporting both undefined (but executed/will execute callback) and Promise as return values for eval, I don't see how it can be backward compatible for current code that does neither.

@guybedford
Copy link
Contributor

So because the invariant of per-line processing requires all eval implementations to conform, there's no middle ground here on creating a backwards compatible subset, got it.

In that case moving ahead with this as-is seems fine to me.

@guybedford
Copy link
Contributor

One other question - what will be the outcome for eval implementations that do not call the callback? Will it just stall?

@ejose19
Copy link
Contributor Author

ejose19 commented Jul 16, 2021

One other question - what will be the outcome for eval implementations that do not call the callback? Will it just stall?

Correct, for most commands that will be the outcome, unless other event resets the processing property to false (like domain errors or SIGINT)

@guybedford
Copy link
Contributor

@ejose19 another option could be to check the function length of the eval function to see if its length includes the callback argument position. If not included it could be assumed to be "sync".

@ejose19
Copy link
Contributor Author

ejose19 commented Jul 16, 2021

@guybedford Ok, that's an interesting approach, but it's enough for this to be considered non breaking? What about consumers that defined the argument but never called it?

@ejose19
Copy link
Contributor Author

ejose19 commented Jul 16, 2021

I tried with that, and it seems this line:

self.eval = self._domain.bind(eval_);

makes the function to have a different length than the original (0 vs 4 for defaultEval), so even if we compare against eval_ directly, I don't think it's safe to "assume" the consumer won't execute the callback based on that alone, in case they're doing something similar to node before passing the function to repl creation.

@guybedford
Copy link
Contributor

Ok, thanks for hearing me out on these questions, agreed the major is best then with the callback being enforced.

@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 24, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 24, 2021
@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 19, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 19, 2021
@nodejs-github-bot
Copy link
Collaborator

@jasnell
Copy link
Member

jasnell commented Dec 24, 2021

The CI failures here need to be investigated. They appear they might b relevant to the change

@jasnell jasnell removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 24, 2021
@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 14, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 14, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

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. 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.

REPL is evaluating new lines before finishing execution of previous ones
9 participants