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

Add unicode handling capability #106

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

lenianiva
Copy link

Previously, if the client program outputs unicode, the unicode output would be garbled when it is piped through NBReader. For more detail, see #105. Writing doesn't seem to have any trouble as demonstrated by examples/cat.rs.

Right now the unicode encoder is always on. In pexpect, an encoding option is available to toggle between the two, which I'm not sure where to put here.

@lenianiva
Copy link
Author

The current behaviour is to withhold a half-completed unicode char from the output buffer. If the client program outputs EOF when the unicode char buffer is incomplete, the char is swallowed. However one problem is that if the client program outputs an invalid unicode char and then valid unicode chars, the output buffer will be stalled and no more chars will be piped into the buffer.

@matthiasbeyer
Copy link
Member

Thanks for your patches and also for your bug report! ❤️ Highly appreciated!

The CI failure should be fixed after #107 is merged, don't worry about that.

I am not sure what the way forward is yet. I like the first option you proposed in #105 most:

Change the type of PipedChar(u8) to PipedChar(char): If the program sends over half of a unicode char and then stop it would hang the reader

But that might break some downstream users for non-UTF8 outputting programs, as I understand it!? @petreeftime maybe you have some ideas what's the best way to go here? Maybe make the whole thing configurable (a flag that can be passed to the library for each call)?

@lenianiva
Copy link
Author

lenianiva commented Aug 17, 2023

Thanks for your patches and also for your bug report! ❤️ Highly appreciated!

The CI failure should be fixed after #107 is merged, don't worry about that.

I am not sure what the way forward is yet. I like the first option you proposed in #105 most:

Change the type of PipedChar(u8) to PipedChar(char): If the program sends over half of a unicode char and then stop it would hang the reader

But that might break some downstream users for non-UTF8 outputting programs, as I understand it!? @petreeftime maybe you have some ideas what's the best way to go here? Maybe make the whole thing configurable (a flag that can be passed to the library for each call)?

I don't think the first option is the best one. The solution in this PR is kind of a compromise between the first two options. This is because the underlying datatype piped by the client program is always u8 and not some char type, and any assembly of u8 into chars is only really needed when downstream needs to check the existence of certain chars to e.g. break line or search a regex.

TLDR: Unicode encoding necessarily runs the risk of stalling the buffer, and this happens either in the read thread or the write thread.

You're correct that this may break non-UTF8 outputting programs, but if the program restricts its output to the ASCII range there shouldn't be any compatibility issue. I left a flag in the code to represent some external option which toggles between unicode and non-unicode encoding.

Do you think it would be sensible to add it as a crate-level feature? Or embed the option in every instance of NBReader and hence spawn (this is the solution from pexpect)?

@matthiasbeyer
Copy link
Member

If anything I'd have it as an option on the data type, not a crate-level feature or compiletime option, because of the simple fact that someone might have UTF-8 outputting programs and non-UTF-8 outputting programs in one project. In general I think compiletime features should never be used for "either-or" functionality 😆

So a flag on the type would be totally fine with me, with UTF-8 compatible reading as the default.

I'd like to see what @petreeftime thinks as well though.

@lenianiva
Copy link
Author

If anything I'd have it as an option on the data type, not a crate-level feature or compiletime option, because of the simple fact that someone might have UTF-8 outputting programs and non-UTF-8 outputting programs in one project. In general I think compiletime features should never be used for "either-or" functionality 😆

So a flag on the type would be totally fine with me, with UTF-8 compatible reading as the default.

I'd like to see what @petreeftime thinks as well though.

If you have any idea for where to add such a flag I can do it in this PR.

By the way why is Pull Request Checks failing? Is it because I didn't sign my commits?

@matthiasbeyer
Copy link
Member

By the way why is Pull Request Checks failing? Is it because I didn't sign my commits?

Its because you didn't signoff your commits (the -s flag in git-commit). You can fix this by doing something like git rebase $(git merge-base origin/master HEAD) -x "git commit --amend --no-edit -s" (there might be a more elegant way... 🤷)

@matthiasbeyer
Copy link
Member

We merged #107 that should make your compile issues go away, please do rebase to latest master.

@lenianiva lenianiva force-pushed the unicode branch 2 times, most recently from fd91788 to f5a333d Compare August 17, 2023 14:40
@lenianiva
Copy link
Author

lenianiva commented Aug 17, 2023

We merged #107 that should make your compile issues go away, please do rebase to latest master.

Rebased. I added a field for NBReader which contains the encoding. The encoding cannot be set via a setter because by the time the setter is called the receiving thread may have already received one or two chars, so the encoding has to be known by the time NBReader::new is called. Whether you would like this function argument to NBReader::new to be rippled to everything else that uses NBReader::new is your call.

I think the problem about the client program stalling the buffer should not be an issue that needs handling. If the client program outputs a invalid unicode char followed by valid unicode then it is not outputting unicode anyways so UTF8 mode is inadequate.

Copy link
Member

@matthiasbeyer matthiasbeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks good.

I think having the encoding be an argument on new, but also providing two convenience helpers NBReader::ascii() and NBReader::utf8(), which call new with the respective argument would be nice and not that much of a hassle.

@petreeftime pinging you again, hope you can have a look as well.

src/encoding.rs Outdated Show resolved Hide resolved
Signed-off-by: Leni Aniva <v@leni.sh>
Signed-off-by: Leni Aniva <v@leni.sh>
Signed-off-by: Leni Aniva <v@leni.sh>
Signed-off-by: Leni Aniva <v@leni.sh>
@lenianiva
Copy link
Author

Overall this looks good.

I think having the encoding be an argument on new, but also providing two convenience helpers NBReader::ascii() and NBReader::utf8(), which call new with the respective argument would be nice and not that much of a hassle.

@petreeftime pinging you again, hope you can have a look as well.

Have you decided that this is the way to go? I need this crate in production

@lenianiva
Copy link
Author

lenianiva commented Mar 1, 2024

I've merged master into this branch and propagated encoding into the new Options structure. By default it still uses ASCII encoding everywhere. However I'm getting this error:

---- session::tests::test_bash stdout ----
thread 'session::tests::test_bash' panicked at 'assertion failed: `(left == right)`
  left: `"/tmp\r\n"`,
 right: `"\u{1b}[?2004l\r\r\n/tmp\r\n\u{1b}[?2004h"`', src/session.rs:543:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

it seems like bash is generating some extra outputs? This also occurs on the master branch.

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.

2 participants