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.

Sign up for GitHub

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

Jump to bottom

Fix polling in WasiPipe #3289

Closed
wants to merge 4 commits into from
Closed

Fix polling in WasiPipe #3289

wants to merge 4 commits into from

Conversation

fschutt
Copy link
Contributor

@fschutt fschutt commented Nov 8, 2022

@john-sharratt: the polling functionality you have implemented won't work as its not
actually polling the receiver - you need to implement it like this:
https://github.com/john-sharratt/wasmer/blob/fe14f086808431d6f64b540421798ac779cdce0a/lib/wasi/src/state/pipe.rs#L223

Note: I've converted wasmer-os over to wasmer and now using WasiPipe hence its important this is implemented correctly. Especially as bash uses Polling on stdin which is connected via this pipe.

@fschutt
Copy link
Contributor Author

fschutt commented Nov 8, 2022

TODO: when this is done and bash / dash is working, we can test it with echo blah | cat

theduke
theduke reviewed Nov 11, 2022
View reviewed changes
Copy link
Contributor

@theduke theduke left a comment

Choose a reason for hiding this comment

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

Not familiar enough with this code to provide a comprehensive review.

Did notice one problematic pattern, though.

lib/wasi/src/state/pipe.rs
let buf_len = buf.len();
if buf_len > 0 {
let reader = buf.as_ref();
let read = read_bytes(reader, memory, iov).map(|_| buf_len as usize)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like you are throwing away the actual amount of bytes read, which can be lower than the buffer size.

lib/wasi/src/state/pipe.rs
if buf_len > 0 {
if inner_buf.len() > buf.len() {
let mut reader = inner_buf.as_ref();
let read = reader.read_exact(buf).map(|_| buf.len())?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing as above.

lib/wasi/src/state/pipe.rs
return Ok(read);
} else {
let mut reader = inner_buf.as_ref();
let read = reader.read(buf).map(|_| buf_len as usize)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

As above.

@theduke
Copy link
Contributor

theduke commented Dec 22, 2022

This will be subsumed by #3422 .

@theduke theduke closed this Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theduke theduke theduke left review comments

@john-sharratt john-sharratt Awaiting requested review from john-sharratt

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@fschutt @theduke