Background process (eldoc) cancels user commands

[vspinu]

Send the following and position your cursor inside abc(|) in order to trigger eldoc. At some point there should be a short hang and the last statements won't be executed.

Sys.sleep(5)
{Sys.sleep(.1); cat("x\n")}
{Sys.sleep(.1); cat("x\n")}
{Sys.sleep(.1); cat("x\n")}
{Sys.sleep(.1); cat("x\n")}
{Sys.sleep(4); cat("x\n")}
cat("This one is canceled \n")
abc()

This is related to #1109 or maybe the same, but here the reason is clear. ESS cannot reliably detect if the process is busy, starts eldoc and then aborts the process.

Needless to say, this is very severe. Now users can never be sure if their multi-line input is fully executed.

I see two potential solutions: 1) send a noop "\n" to the process before every command, wait a bit and if process is ready proceed with the command 2) move all background processes to proper async commands

1 is a hack, 2 is a bit more work and it's not guaranteed to be without caveats.

@lionel-

[lionel-]

Sending noop newlines is a bit hacky, I regularly get empty prompts in the console because of the newlines we currently send.

I think this would be better solved by the async queue of pending evaluation of #1017. If the eval queue is not empty then background processes would be disabled. There could be race conditions but they should be rare with this setup.

It would still be useful to have getOption("ide.prompt") and getOption("ide.continue") in base R. @mmaechler Sorry to insist but I hope you'll reconsider including this feature in R. Currently IDEs based on text interaction don't have much help from R compared to IDEs that embed R with the C API.

[mmaechler]

Sending noop newlines is a bit hacky, I regularly get empty prompts in the console because of the newlines we currently send.

me too (and in some cases, it happens so often I think it's very embarrassing)

I think this would be better solved by the async queue of pending evaluation of #1017. If the eval queue is not empty then background processes would be disabled. There could be race conditions but they should be rare with this setup.

It would still be useful to have getOption("ide.prompt") and getOption("ide.continue") in base R. @mmaechler Sorry to insist but I hope you'll reconsider including this feature in R. Currently IDEs based on text interaction don't have much help from R compared to IDEs that embed R with the C API.

I only vaguely remember. .. you had argued one needed more than the "prompt" and "continue" options Can you remind me where this was proposed / discussed?

[lionel-]

We discussed this in various threads, most recently in #1008 I think (with an implementation in #1018). We also discussed it in person in Toulouse. The gist is that we could reliably detect that R is ready for background commands if we had robust prompt detection. Using a naked getOption("prompt") is too brittle because normal outputs may include >. Using a tagged prompt is too intrusive because the control sequence might end up in unexpected places like log files via cat(getOption("prompt")). To avoid this, "ide.prompt" would allow ESS to set a prompt tag that would never be consulted by user code.

[vspinu]

Guys, we need to address this. It's a huge handicap. I would propose to disable the process interrupt within the ess-command for now till we figure it out. Also set the timeout to infinity.

[lionel-]

I would propose to disable the process interrupt within the ess-command for now till we figure it out.

This is the stopgap approach initially implemented. The problem is that this makes typing or moving the cursor very choppy when background evaluation is enabled with slow remotes. To make it worse, it also prevents user C-g from working.

Also set the timeout to infinity.

Are you still having timeout issues?

[vspinu]

I have set timeout to infinity. At least now, when emacs stalls and the user waits without C-g-ing the user's own computation is guarantied to complete. I will try to find time to think more deeply about the issue latter this month.

@lionel-

Are you still having timeout issues?

The timeout issue is as I have described above: long R evaluation starts, at some point eldoc triggers and emacs stalls. After 30 seconds both eldoc and original user's computation is canceled.

[lionel-]

Here is how I propose to resolve this issue:

Counting prompts

For each line sent to R, we either get a secondary prompt + or a primary prompt >. This means that interaction with R can be conceived in terms of a request-response protocol where requests are lines of code and responses are delimited by prompts, optionally preceded by a payload (parse errors or R output).

This means that we can improve our busy detection by increasing a count for each line sent to the process and decreasing it for each prompt detected in the output. If the count is not zero, we know for sure that R is busy.

Reliable handshaking

The opposite isn't true however. If the count is zero, R might still be busy. That will happen when a line that looks like a prompt was sent by R code instead of the REPL. So we still need a handshake. We can improve handshaking in two ways:

• We send the handshake to the C-level stdout using the same approach taken in .ess.command(). This way handshakes will never end up in sinked output and ESS is able to detect them and remove them of the output. We can also use the trick used in .ess.command() to preserve .Last.value.

• Instead of an empty line we send a string like "ESS-HANDSHAKE", possibly starting with a control character. This way if we receive the response later on we can reliably detect our response and not some other output. As long the handshake wasn't received, we know that R is busy.

Longer term: message framing by the REPL

I believe that the combination of prompt counting and improved handshaking will greatly improve the reliability of ESS/R interaction. It'll still not be 100% proof though. The worst failure mode I can think of is when the user calls reticulate::repl_python() and ESS thinks R is not busy by detecting the python prompt >>>.

For the longer term, I'm thinking about proposing a global option to instruct R to message-frame responses to lines of input. Something along the lines of https://github.com/lionel-/r-source/commit/8a2811e6b06e37f77ee1caac7c2a0ce5188d34ad. This way we'll be able to coordinate with 100% reliability the lines of input with a given output. In particular, we'll be able to pipe input to R and match input and output in the R console as in visible evaluation.

[vspinu]

It remains to be seen if it works. I am so tired of this prompt hunting that I am skeptical of an "improved version" of it.

IMO it would be much simpler to go with the async RPC and leave the prompts and "bussiness" alone. Not a full proof solution but clean and relatively reliable.

[lionel-]

I am confident that improved handshaking will solve the issue robustly and cleanly.

IMO it would be much simpler to go with the async RPC

What is the async RPC approach?

[vspinu]

It's already implement (50 lines of code ).

In the example of eldoc, send a request through stdin. Even if the process is busy it will accumulate and execute at the end. On the other end, process the request and put a delimited output on stdout. Process filter parses the message and if the eldoc context is still the same (aka pointer is on the same object) show eldoc, otherwise discard the message.

To my mind this is very simple and hustle free aproach, no prompts no busy waiting etc.

[lionel-]

How do we prevent sending inputs in unsafe contexts, i.e. when R is waiting for more inputs with a secondary prompt? I like the idea of async commands but I think it'll probably have to be implemented on top of busy detection.

[vspinu]

Indeed. We don't send when it's busy, but even if we send, it will be only in those cases with intermediate prompts on long inputs. Those contexts are safe. I don't think we have other cases when we fail to detect business.

An extra layer of protection can be an \n ping before sending the command.

[lionel-]

Those contexts are safe.

Not necessarily because there might be incomplete input sent to R at that point. This is why prompt counting would help making things even more robust. But yes, detecting primary prompts gets us a long way there.

An extra layer of protection can be an \n ping before sending the command.

I see two problems with newline handshakes. The most problematic is that output might currently be sinked to a file by the user, causing disruptive newlines in their sinked output. And actual user output might be confused with a newline response in edge cases.

By the way, I spent a large part of my winter break thinking about business detection, improved handshaking, and working on an implementation. I would like to finish this work. It might be worthwhile to think about moving eldoc and completion to an async implementation, this might improve typing performance on slow connections, even with the new Emacs cancellation mechanism on user input. However this is a major change and it's probably better to wait until we release ESS before making that switch.

[vspinu]

Not necessarily because there might be incomplete input sent to R at that point.

I don't think so. What we have so far should be enough. The moment an input is sent we mark process as busy, so incomplete input should always result in busy state.

The most problematic is that output might currently be sinked to a file by the user,

Hmm. Good point.

and working on an implementation.

Great. Hopefully it's not very complex :wink:

The current status of interrupting the process on ess-command is quite unsatisfactory. I would suggest that we drop it before the release.

However this is a major change

It doesn't look at all to me, but one never knows. In terms of implementation it's quite simple. Let me also give it a try and see how it works for a month or two. My main annoyance is not even with eldoc or completion, but with the work-dir synchronization that runs on a timer. It's the cause of the spilling into the process buffer and ghost prompts that people report.

[lionel-]

The moment an input is sent we mark process as busy, so incomplete input should always result in busy state.

Yeah but we might receive a primary prompt in the meantime. We are pipelining input and arbitrary regions, there might be an intermediary complete expression that takes time to execute, followed by incomplete input. I can construct a reprex if you like.

The current status of interrupting the process on ess-command is quite unsatisfactory. I would suggest that we drop it before the release.

I've been working on this for a couple of years at this point!

[lionel-]

To be clear, if async eldoc, completions, and dir-sync prove to work robustly let's go for it (and as I've argued above, I think improved handshaking will be important for this). I just don't think we should drop sync commands altogether.

[vspinu]

We are pipelining input and arbitrary regions, there might be an intermediary complete expression that takes time to execute, followed by incomplete input.

You are right. I am getting rusty :chart_with_downwards_trend: I think we discussed this before (maybe not once). The solution is to either not to allow incomplete expressions to start with, or to nail the business detection.

I just don't think we should drop sync commands altogether.

By dropping I meant disabling only the interruption of the process on quit (the stuff added last year). It can be optional for the invocator through a parameter. We re-enable it when you figure out the business part, if it at all possible.

Or maybe for now we just introduce \n ping before all commands. Then process interruption is guaranteed to interrupt only our commands. In case of false positives newline is uniformly better than an arbitrary command. It can be a good tradeoff. WDYT?

[lionel-]

The solution is to either not to allow incomplete expressions to start with

Emacs is moving towards tree-sitter for which Jim implemented an R backend, but I feel uneasy about relying on complete expression detection for REPL interaction. First because, as you've argued elsewhere when I was trying to send input line by line, this would prevent pipelining long inputs to R (e.g. giant functions such as [.data.table). More importantly, even though the R language doesn't evolve quickly, it does evolve. And then we need to detect the R version, make sure we parse the correct version of the language, and there shouldn't be any bugs. It seems to be a bit messy overall (though arguably not more than busy detection), and in any case a long term project.

By dropping I meant disabling only the interruption of the process on quit (the stuff added last year). It can be optional for the invocator through a parameter. We re-enable it when you figure out the business part, if it at all possible.

I really want to nail this down and I've mobilised all my free time for this in the last two years (I don't have much free time at the moment but this is still weeks of work). I'd like to finish this for the next release which I currently aim for April, around R 4.2. If the improved handshaking doesn't work, I'll revise this goal.

In general I don't think it's reasonable to prevent interruption during sync commands. The user and Emacs (on-typing interrupts) should be able to C-g at any time when R is unresponsive or slow to respond. If you'd like, we could make interrupt catching optional as a stopgap and perhaps the default until this is figured out.

Or maybe for now we just introduce \n ping before all commands. Then process interruption is guaranteed to interrupt only our commands. In case of false positives newline is uniformly better than an arbitrary command. It can be a good tradeoff. WDYT?

I do think handshaking is key to solve this without further help from R, but the current newline handshakes have problems that I mentioned above which I'd like to solve.

[vspinu]

I do think handshaking is key to solve this without further help from R, but the current newline handshakes have problems that I mentioned above which I'd like to solve.

Sure, take as much time as you need. I am not proposing newline as a permanent solution. Just as a temporary fix for now. We are sending commands anyhow, so sink problem is in place right now regardless. I like the interruption, but I don't like that it interrupts user commands together with our commands. It burns me badly on long tasks. The main problem being that I don't know which part was executed and which not so the entire job has to be restarted.

[lionel-]

Responding to myself:

this would prevent pipelining long inputs to R

Likely not an issue since parsing will be instantaneous. But we'd need to pipeline multiple complete expressions at the same time without waiting the response for performance. And we'd probably need to send incomplete inputs to R as well unless we want to reimplement a REPL on top of R with secondary prompts, parsing error reporting etc.

The main thing this approach would bring is that we'd know for sure whether R is in an incomplete input state or not. And so whether it's safe to send an async command without having to wait for R being non-busy.

I am not proposing newline as a permanent solution. Just as a temporary fix for now.

That sounds good.

[vspinu]

First because, as you've argued elsewhere when I was trying to send input line by line, this would prevent pipelining long inputs to R (e.g. giant functions such as [.data.table).

We might not be talking about the same thing :thinking: What I meant is to not even allow the user to accumulate incomplete input. Basically change the evaluation commands to work on complete expressions. No paragraphs, lines etc.

Too much of a change I know, but maybe it's a good time to bootstrap everything from zero. Preserving backward compatibility and piling on old code takes too much time ... but well maybe for another topic.

[lionel-]

We might not be talking about the same thing

We were, I was just confused. Cf my last message, we just need to be sending multiple complete expressions without waiting for responses to solve the perf issue.

Basically change the evaluation commands to work on complete expressions

We probably can't do that because of all the complications this would bring (secondary prompts, parsing errors), cf last message.