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

Avoid manual polling #231

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Avoid manual polling #231

wants to merge 2 commits into from

Conversation

tamird
Copy link
Contributor

@tamird tamird commented Dec 17, 2024

See commit message.

@tamird
Copy link
Contributor Author

tamird commented Dec 17, 2024

r? @djc

@djc
Copy link
Member

djc commented Dec 17, 2024

Can you add some high-level context/motivation here? Is it feasible to split this in smaller changes (that still compile/pass tests)?

1 similar comment
@djc
Copy link
Member

djc commented Dec 17, 2024

Can you add some high-level context/motivation here? Is it feasible to split this in smaller changes (that still compile/pass tests)?

@tamird
Copy link
Contributor Author

tamird commented Dec 17, 2024

Updated the commit message.

@tamird
Copy link
Contributor Author

tamird commented Dec 17, 2024

Sorry, didn't address your question. No, I don't think it's feasible to break this into smaller bits.

Slices already encode their lengths; use that instead.

Remove the unused return value while I'm here.

Closes console-rs#225.
Rather than manually implementing non-blocking reads using polling,
simply configure the underlying file descriptor to be non-blocking where
necessary. Replace libc calls with the normal abstractions from the
standard library.

This makes the code less error prone and properly encodes type system
invariants (such as `io::Read::read` requiring its receiver to be
mutable) which were previously not preserved because the implementation
used raw file descriptors.
@djc
Copy link
Member

djc commented Dec 18, 2024

Sorry, this is a larger change than I feel comfortable reviewing (since I have limited experience with this crate and the underlying Unix APIs). The transformation from select to temporarily changing the socket to be non-blocking seems pretty tricky to get right and I don't think the motivation is sufficient given the risk.

@tamird
Copy link
Contributor Author

tamird commented Dec 18, 2024

Who would be an appropriate reviewer? @atanunq wrote the original code in c8ea46f and @mitsuhiko modified it in #169.

@djc
Copy link
Member

djc commented Dec 18, 2024

My impression is that @mitsuhiko doesn't have much capacity for console in the near future.

@tamird
Copy link
Contributor Author

tamird commented Dec 18, 2024

The original code written by @atanunq borrowed from rustyline, which uses polling, but rustyline also uses a non-zero timeout. I'd love for one of these two original authors to weigh in.

Also tagging @pksunkara who approved the original changes.

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