wasi: Test that pollables may be used multiple times#7763
Conversation
|
@pchickey and @guybedford, does this look like enough to have caused a test failure in the jco work? |
|
Unfortunately I tried this in JCO and it is definitely passing, so I don't think this is capturing the case. JCO pollables will only ever resolve once as they are promise-based, after which they will be always-resolving. @elliottt let me know if I can help debug further at all. |
| stdout_pollable.block(); | ||
| assert!(stdout_pollable.ready(), "after blocking, pollable is ready"); | ||
|
|
||
| let n = stdout.check_write().unwrap() as usize; |
There was a problem hiding this comment.
Perhaps this should write until check write doesn't allow anymore writing to ensure it's hitting backpressure?
The issue here in JCO is that we don't have backpressure for stdout since Node.js doesn't support it (https://nodejs.org/docs/latest/api/process.html#a-note-on-process-io).
There was a problem hiding this comment.
I'm working separately on a test that pipes stdout to stdin of another instantiation of the same component, and I think will give us a lot more control over how this test works. I'll ping you when it's ready :)
elliottt
force-pushed
the
trevor/pollable-reuse-test
branch
from
131aca3 to
bd0d40a
Compare
elliottt
force-pushed
the
trevor/pollable-reuse-test
branch
from
bd0d40a to
cd1398b
Compare
elliottt
requested review from
guybedford and
pchickey
and removed request for
a team,
guybedford and
pchickey
| if !producer.success() { | ||
| // make sure the consumer gets killed off | ||
| if consumer.try_wait().is_err() { | ||
| consumer.kill().expect("Failed to kill consumer"); |
There was a problem hiding this comment.
rather than kill the consumer, can we tell each the producer and consumer how many bytes or messages to send/expect total, so they can both exit gracefully once those conditions are met? this will also let us assert, in the consumer, that after everything expected is consumed, the pollable does not become ready "ever again" (poll with a 250ms timeout and call it good)
There was a problem hiding this comment.
If the producer panics, we'll still want to kill the consumer. This was more about ensuring that we cleanup failures properly than anything else.
There was a problem hiding this comment.
Additionally, consumed amount is something that we could compute in the individual piped tests, and I think would be easier to debug as an assertion failure in the consumer side.
| // Below here is mechanical: there should be one test for every binary in | ||
| // wasi-tests. | ||
| #[test] | ||
| fn piped_simple() { |
There was a problem hiding this comment.
Just curious why you chose to create a new prefix and test for these test programs and tests? or can we just make them part of the cli tests?
There was a problem hiding this comment.
These are slightly different, as they require two instances to be setup. I suppose they could be cli tests, but it seemed cleaner to separate them out, given that they have special run requirements.
3 participants
We didn't have a test that covered using a pollable multiple times in the test-programs, despite relying on that behavior in the preview1 adapter.