Skip to content

After calling setupHistory, the program continues to run even after the REPL is closed. #52386

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

Closed
LiST-GIT opened this issue Apr 6, 2024 · 6 comments · May be fixed by #52587
Closed

After calling setupHistory, the program continues to run even after the REPL is closed. #52386

LiST-GIT opened this issue Apr 6, 2024 · 6 comments · May be fixed by #52587
Labels
confirmed-bug Issues with confirmed bugs. repl Issues and PRs related to the REPL subsystem.

Comments

@LiST-GIT
Copy link

LiST-GIT commented Apr 6, 2024

Version

v20.5.0

Platform

macos

Subsystem

No response

What steps will reproduce the bug?

const repl = require('repl');

var r = repl.start({});
r.setupHistory(__dirname + '/history', () => { }); // Different results will occur here
r.close();

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior? Why is that the expected behavior?

No response

What do you see instead?

After calling setupHistory, the program continues to run even after the REPL is closed.

Additional information

No response

@cola119 cola119 added confirmed-bug Issues with confirmed bugs. repl Issues and PRs related to the REPL subsystem. labels Apr 6, 2024
@lucshi
Copy link

lucshi commented Apr 17, 2024

Hi @LiST-GIT, according to the document, the rl.close() method closes the Interface instance and relinquishes control over the input and output streams. When called, the 'close' event will be emitted. Calling rl.close() does not immediately stop other events (including 'line') from being emitted by the Interface instance. IMO, rl.close() does not exit the process.

In the test code, var r = repl.start({}); opens stdin and stdout and wait for users input so closing the REPL server does not exit the stdin and stdout unless users send EXIT event.

I wrote 2 samples to make the test code work smoothly:

const repl = require('repl');
const path = require('path');

// Start the REPL session
const r = repl.start();


r.setupHistory(path.join(__dirname, '/history'), () => {});

r.close();

r.on('exit', () => {
    console.log('REPL session closed.');
    process.exit();
  });

It quits the node process very soon without giving users chance to input and save into history.
Below modifications let users input and save history and exit by inputing ".exit" in the repl.

const repl = require('repl');
const path = require('path');

// Start the REPL session
const r = repl.start({
  prompt: '> '  
});

// Setup the history for the REPL session
r.setupHistory(path.join(__dirname, '/history'), (err) => {
  if (err) {
    console.log('Error setting up history:', err);
  } else {
    console.log('History setup completed.');
  }
});

// Define the exit event handling
r.on('exit', () => {
  console.log('REPL session closed.');
  process.exit();
});

@lucshi
Copy link

lucshi commented Apr 17, 2024

If agreed, would I submit a PR to explain the repl.close() behavior more clearly especially for stdin and stdou cases instead of file stream cases?

@LiST-GIT
Copy link
Author

Thank you for your response @lucshi . I've updated the code again. In the callback of setupHistory, calling r.pause can now properly exit the program. I think it might be because r.close already calls r.pause, but in setupHistory, due to the asynchronous fd executing the open callback, r.resume is called again, which is why it continues to wait on stdin. Is this by design?

const repl = require('repl');

var r = repl.start({});
r.setupHistory(__dirname + '/history', () => {
    r.pause();
});
r.close();

@lucshi
Copy link

lucshi commented Apr 17, 2024

According to the REPL implementation , setupHistory resumes the REPL again even if it has been closed. IMHO, in real pracise, users (programmers) ought to know the callback of setupHistory occurs after r.close() so the original code snippet does not make great sense although it is a great corner test case. Usually users are supposed to do some evaluations to well use REPL after history setup. A common code snippet found on web be like :

const repl = require('repl');
const fs = require('fs');

// Start the REPL
const myRepl = repl.start({
  prompt: '> '
});

// Setup history file
const historyFilePath = __dirname + '/history.txt';
myRepl.setupHistory(historyFilePath, () => {
  console.log('History setup complete.');

  // Execute some commands after history setup
  myRepl.eval('console.log("Executing command 1")', myRepl.context, 'history_callback', (err, result) => {
    if (err) {
      console.error('Error executing command 1:', err);
    } else {
      console.log('Command 1 executed successfully.');
    }
  });

  myRepl.eval('console.log("Executing command 2")', myRepl.context, 'history_callback', (err, result) => {
    if (err) {
      console.error('Error executing command 2:', err);
    } else {
      console.log('Command 2 executed successfully.');
    }
  });

  // Close the REPL after executing commands
  setTimeout(() => {
    console.log('Closing the REPL...');
    myRepl.close();
  }, 2000); // Close the REPL after 2 seconds
});

// Event handler for when REPL is closed
myRepl.on('close', () => {
  console.log('REPL closed.');
});

As the REPL API manuals does not give such a clear sample to demostrate how to close the REPL gracefully. I think complementing the document could be a quick solution. What's your advice here?

@LiST-GIT
Copy link
Author

I think you're right, the programmer should determine the order of setupHistory and close themselves.

lucshi pushed a commit to lucshi/node that referenced this issue Apr 18, 2024
Emphasize "setupHistory" could avoid repl from closing and users
shall take care of repl closing in appropriate methods.

Fixes: nodejs#52386
@lucshi
Copy link

lucshi commented Apr 19, 2024

I think I can submit a PR to resolve this issue even if the close is done ahead of setupHistory.
Feel free to reopen this issue @LiST-GIT

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. repl Issues and PRs related to the REPL subsystem.
Projects
None yet
3 participants