Skip to content

Extend the polling feature to cover TryStream::try_read_inner()#76

Open
daniel-thompson wants to merge 3 commits into
zhiburt:mainfrom
daniel-thompson:main
Open

Extend the polling feature to cover TryStream::try_read_inner()#76
daniel-thompson wants to merge 3 commits into
zhiburt:mainfrom
daniel-thompson:main

Conversation

@daniel-thompson

Copy link
Copy Markdown

The busy-loop when waiting for characters to be available is consuming far too much CPU (and power) for my use case.

These three patches introduce a new method to the NonBlocking trait making it possible to wait until data is ready. An "always ready" implementation is provided by default and should prevent this from being a breaking API change (although to use the new feature then code must be changed).

On Unix systems we fully implement NonBlocking::ready() and then use if from sync_session.rs to avoid busy-waiting.

The ready() method allows us to optionally perform a sleeping poll to
determine if characters are available. This can be used, on platforms
that support polling, to minimise CPU time when waiting for characters.
@zhiburt

zhiburt commented Apr 1, 2026

Copy link
Copy Markdown
Owner

Hi @daniel-thompson

I like idea to add a timeout to read function.
But as I remember try_read was meant to work as it works now.
Meaning juts simple "try" nothing more.

Maybe adding a new method would be better?
Something like try_read_until, not the best name but something :)
Or adding a timeout argument Option<Duration> to try_read itself (maybe it's even better).

What do you think?

PS: And it would be great to turn CI green :)


The busy-loop when waiting for characters to be available is consuming far too much CPU (and power) for my use case.

I can imagine if you are doing try_read.

In theory you could use combination of Session::set_expect_timeout and session.expect(NBytes(1)).

But I see the problem that sometimes reading with timeout would be just easier.
But I believe we either shall add an argument or a new function.

@daniel-thompson

Copy link
Copy Markdown
Author

Reading things back I'll probably just drop the changes to try_read() entirely. I added it because I thought there were code paths from expect() to try_read() but it looks like that was a mistake... looking again I can't find such a code path.

PS: And it would be great to turn CI green :)

Could you kick of a CI run on the main branch? There are some failures caused by my changes but some look like they were already there (the Code Coverage / Coveralls check for example).

ready() has a default "always-ready" implementation but we Unix systems
we can use a Poller to make character polling more efficient. Let's do
that!
Currently TryStream::try_read_inner() are part of a busy-loop that waits
for characters. We can make that wait use significantly less CPU by
sleeping until characters are available instead.

This should have no impact on performance providing characters or EOF
are received before reaching the timeout. The 100ms timeout could impact
the responsiveness of Sessions with a short timeouts. Happily these
sessions should be uncommon and their needs could be met by disabling
the polling feature.
@daniel-thompson daniel-thompson changed the title Extend the polling feature to cover TryStream::try_read() and TryStream::try_read_inner() Extend the polling feature to cover TryStream::try_read_inner() Apr 2, 2026
@daniel-thompson

Copy link
Copy Markdown
Author

Sorry for the close/reopen... it happened automatically when I removed my changes from my main branch to check the CI results without any of my changes.

I have fixed the clippy warning that came from my changes but I'm not sure how best to fix the existing CI problems on MacOS and with the code coverage tool.

@daniel-thompson

Copy link
Copy Markdown
Author

I've reviewed the failures in the CI report for this branch and am confident none are introduced by the changes in this PR.

The two macos-latest failures are observed in runs that do not contain my changes (https://github.com/daniel-thompson/expectrl/actions/runs/23892372808 ). The same is true of the code coverage checks (https://github.com/daniel-thompson/expectrl/actions/runs/23892372814).

The main check is a credit check failing and I don't think any code from the PR has been downloaded at the point of failure.

@zhiburt

zhiburt commented Apr 18, 2026

Copy link
Copy Markdown
Owner

Sorry for delay once again.

I think we can land it.

The only 2 questions I have:

  1. Maybe add a configurable parameter for this timeout? (just add a setter and pass value throughout structs). And by default it's probably must be None or maybe even no?
    Specifically we have one case where expect_timeout can be less then 100milis, in theory.

  2. Just a thought (can be ignored). Maybe if we would change the interface NonBlocking+Stream we could read more effectively (https://github.com/daniel-thompson/expectrl/blob/be5635b43904fb8901f4e5e9e47b5e935e1fbc0c/src/session/sync_session.rs#L517-L523) (maybe not).
    Specifically I am saying about setting non blocking reading and offseting it. Maybe it's more effective to use polling::* for reading right away. Just saying, maybe it's possible.
    Or maybe we even don't need non_blocking mode if we know the data is there? Will read call be blocking without it?

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