:pipe hangs when piping a large file through sed

[AnActualEmerald]

Summary

Helix freezes and becomes unresponsive when piping a large (~7600 lines) file into sed. Piping the same file into sed outside of helix doesn't produce the same behavior, and helix seems to handle piping smaller files just fine. Piping into other commands also doesn't seem to cause the same issue though I didn't try very many.

Reproduction Steps

I tried this:

1. hx frankenstein.txt (from here)
2. %
3. |sed s/l/w/g

I expected this to happen:
The command to execute and all of the 'l' characters in Frankenstein to be replaced with 'w'

Instead, this happened:
Helix became unresponsive

Helix log

~/.cache/helix/helix.log

2022-07-21T01:21:42.642 helix_view::theme [WARN] Theme: invalid style attribute: modifier
2022-07-21T01:21:42.642 helix_loader [DEBUG] Located configuration folders: []
2022-07-21T01:21:42.648 mio::poll [TRACE] registering event source with poller: token=Token(1), interests=READABLE | WRITABLE
2022-07-21T01:21:42.648 mio::poll [TRACE] registering event source with poller: token=Token(2), interests=READABLE | WRITABLE
2022-07-21T01:21:42.648 mio::poll [TRACE] registering event source with poller: token=Token(0), interests=READABLE
2022-07-21T01:21:42.648 mio::poll [TRACE] registering event source with poller: token=Token(1), interests=READABLE

Platform

Linux (Pop!_Os)

Terminal Emulator

Alactritty v0.10.0-rc1

Helix Version

helix 22.05 (5ed6223)

[MDeiml]

I'm pretty sure the problem is that

helix/helix-term/src/commands.rs

Lines 4429 to 4433 in 76756f0

	fn shell_impl(
	shell: &[String],
	cmd: &str,
	input: Option<&[u8]>,
	) -> anyhow::Result<(Tendril, bool)> {

takes a byte slice for input, which in this case is the whole file. This means that the internal rope has to be copied completely. shell_impl should probably take impl Iterator<&[u8]> or something similar.

[A-Walrus]

This means that the internal rope has to be copied completely.

I don't think that's the problem. Firstly copying ~7000 lines shouldn't take that long, but secondly it freezes indefinitely and the CPU usage of the helix process goes to 0%, so I don't think this is a performance problem, I think this is some kind of deadlock...

[A-Walrus]

Piping into other commands also doesn't seem to cause the same issue though I didn't try very many.

I was able to reproduce this with grep, as well as sed, but strangely awk seems to work fine...

[kirawi]

As an aside, copying the rope in shell_impl can be avoided by using to_writer from helix-view/src/document.rs.

[MDeiml]

Oh, didn't read that in time. I already fixed it in a different way (I think this should solve it), but making this async and using to_writer might be an idea for the future (no pun intended).

[MDeiml]

Checked and didn't solve it :(

[kirawi]

You can use block_on to avoid making it async. I think it would be a more minimal changeset compared to your PR.

[MDeiml]

I think ultimately this needs to be "real" async or at least multithreaded, since we probably need to write to stdin and poll stdout at the same time. I think that's what causing this issue.

I spent some hours trying to get it to work, but it didn't work out :)

[MDeiml]

Like block_on like you suggested would easily work, it just doesn't solve this problem.

[kirawi]

I think ultimately this needs to be "real" async or at least multithreaded, since we probably need to write to stdin and poll stdout at the same time. I think that's what causing this issue.

I spent some hours trying to get it to work, but it didn't work out :)

There is #3029

[MDeiml]

That looks nice. Some pieces are missing to fix this e.g. we actually need to spawn a new task so execution is actually multithreaded, but it's a start.

:pipe has the added complication that it probably should be blocking. That's the thing I didn't manage to get to work.

[MDeiml]

After more experimentation I managed to get a working version of this, but it seems writing to stdin has to be done in a new thread / async task. But that means that we have to take ownership of the input data so we have to copy all of it.

Intuitively there has to be another solution since we're blocking on the threads execution, so we don't give away any references. But I don't see a way to tell that to the borrow checker other then with unsafe.

[archseer]

But that means that we have to take ownership of the input data

I think that's fine, you can clone the rope + selection and move them into the async closure. Cloning a rope is a shallow copy (essentially cloning an Arc internally), it will reuse the same tree and only start diverging as changes are made to the rope. The structure is immutable

[MDeiml]

Ah ok, thanks! Gonna open a PR then.

[the-mikedavis]

Looks like this is fixed by #3180 but not #3136