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

WIP pipe support #34

Merged
merged 5 commits into from
Feb 19, 2024
Merged

WIP pipe support #34

merged 5 commits into from
Feb 19, 2024

Conversation

willmcgugan
Copy link
Contributor

No description provided.

@willmcgugan willmcgugan merged commit c5ff45c into main Feb 19, 2024
@willmcgugan willmcgugan deleted the pipe branch February 19, 2024 16:30
@youtux
Copy link

youtux commented Feb 22, 2024

I see some issues with this impl (all verified):

  • piping to tl results in the creation of a file that keeps on growing on disk. This can be a big issue when piping a lot of data, or when leaving the program to run for a long time
  • the temporary file is not deleted if the process is killed
  • the subprocess is not terminated when the parent is killed

@youtux
Copy link

youtux commented Feb 22, 2024

is there a way to implement piping without spawning a new process, since that seems to be the cause of all issues after all?
I guess the hard part is to try to tell textual.app.App to read from /dev/tty instead of /dev/stdin, right?

@willmcgugan
Copy link
Contributor Author

If you want to be able to view / search the data that is being piped, it needs to be stored somewhere. Memory is going to run out before disk, so a temp file is the only place for it to go.

By kiling the process, do you literally mean with the kill command? Why use that and not ctrl+C ?

@youtux
Copy link

youtux commented Feb 22, 2024

If you want to be able to view / search the data that is being piped, it needs to be stored somewhere. Memory is going to run out before disk, so a temp file is the only place for it to go.

True, but what you could do is to keep in memory or on disk only a certain amount of data. That's actually what terminal apps do (and you can usually configure the amount of "scrollback", aka the buffer size).

By kiling the process, do you literally mean with the kill command? Why use that and not ctrl+C ?

Of course Ctrl+C is the preferred way, but processed can be killed for other reasons, for example if they use too much memory and the OS kills them. (but there can be many more examples than this).
It's just something to be aware of in general when spawning processes

@youtux
Copy link

youtux commented Feb 22, 2024

I would expect most of the users to prefer having a limited "scrollback" capability rather than having the program causing the system to run out of disk space

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants