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

better support for multithreaded programs #64

Open
BurntSushi opened this issue Sep 5, 2016 · 9 comments
Open

better support for multithreaded programs #64

BurntSushi opened this issue Sep 5, 2016 · 9 comments

Comments

@BurntSushi
Copy link

@Stebalien First off, thank you for maintaining this wonderful library. :-)

I came across a particular issue with the current API. Specifically, TerminfoTerminal::new_with_terminfo takes ownership over a TermInfo. Even though TermInfo is never mutated by a TerminfoTerminal, a TermInfo cannot be reused across multiple TerminfoTerminal values. In particular, the only way to amortize the cost of building a TermInfo is to clone it, but even this can be costly.

I'm not sure what the right API is, but I will note that using Arc solves my specific problem (which permits very cheap multithreaded read-only access).

There are potentially many other solutions, all with their own downsides:

  1. Require Arc<TermInfo> instead of TermInfo, but this feels a little strange to me.
  2. Require <T: AsRef<TermInfo>>. This means TermInfoTerminal will need to grow a type parameter.
  3. Define a new trait with the existing methods on TermInfo and require that. This also requires an additional type parameter.
  4. Make TermInfo an opaque struct and use an Arc internally, which will make cloning trivially cheap.

Other ideas? Thoughts?

@BurntSushi
Copy link
Author

For the time being, I ended up just implementing Terminal with my own type. That allowed me to move forward. See: https://github.com/BurntSushi/xrep/blob/2bda77c414d113b3640b372afffc781a2fe56e6d/src/terminal.rs

@Stebalien
Copy link
Owner

Actually, I've been trying to make a new interface that acts more like std::io::stdin. That is, just have a global "terminal" (although I'd still probably have a way for users to create "mock" terminals for testing). Now that I've submitted my thesis, I'm actually going to make a serious attempt at tackling this. It's not so straight forward because I'd like to support per-thread buffering to avoid crappy interleaved output.

@BurntSushi
Copy link
Author

My particular pattern is, "write output to a String in memory, then later output that String to stdout." Would your revisions still support that?

@BurntSushi
Copy link
Author

(Specifically, I need a way to write the output of some work without having to do synchronization on each call to print something. I'm happy to trade the memory required to do it.)

@Stebalien
Copy link
Owner

One of the feature's I'm working on is the ability to create fully buffered terminal handles that flush on drop. This way, one can build up a nice colored message and have it displayed all at once instead of getting interleaved with messages from other threads.

@mooman219
Copy link

mooman219 commented Oct 24, 2016

Would like to hop on this issue with getting Sync support on StdoutTerminal since it's along the same lines of multithreaded support. I wanted color support in a simple logger and it needed to be Send + Sync. A workaround is just wrapping it in a mutex but that's really overkill.

@BurntSushi
Copy link
Author

@mooman219 I don't think making StdoutTerminal be Sync is related to the specific problem in this ticket, and if one did make it Sync, I imagine it would use a mutex internally anyway. See how Stdout is handled in the standard library, for example.

@Stebalien
Copy link
Owner

I wouldn't make it Sync. Instead, I'd make creating multiple terminal "handles" cheep. Unfortunately, every time I approach this project, I get stumped trying to figure out the best way to do this on Windows. Ideally, I'd like multi-process programs like cargo to not miscolor error messages.

@mooman219
Copy link

mooman219 commented Oct 24, 2016

@BurntSushi Apologies, it felt like it fit under the title and didn't want to little issues on here.

@Stebalien Sounds good to me! I'm quite extremely new to Rust and have been battling it on concurrency lately.

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

No branches or pull requests

3 participants