Wasmtime GitHub notifications bot (Jan 09 2024 at 18:03): Wasmtime GitHub notifications bot (Jan 09 2024 at 18:03):elliottt opened PR #7763 from elliottt:trevor/pollable-reuse-test to bytecodealliance:main:
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.
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
Wasmtime GitHub notifications bot (Jan 09 2024 at 18:04):elliottt commented on PR #7763:
@pchickey and @guybedford, does this look like enough to have caused a test failure in the jco work?
Wasmtime GitHub notifications bot (Jan 09 2024 at 20:08):guybedford commented on PR #7763:
Unfortunately I tried this in JCO and it is definitely passing, so I don't think this is capturing a 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.
Wasmtime GitHub notifications bot (Jan 09 2024 at 20:08):guybedford edited a comment on PR #7763:
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.
Wasmtime GitHub notifications bot (Jan 09 2024 at 20:16):guybedford submitted PR review.
Wasmtime GitHub notifications bot (Jan 09 2024 at 20:16):guybedford created PR review 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).
Wasmtime GitHub notifications bot (Jan 09 2024 at 20:37):elliottt created PR review 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 :)
Wasmtime GitHub notifications bot (Jan 09 2024 at 20:37):elliottt submitted PR review.
Wasmtime GitHub notifications bot (Jan 09 2024 at 23:58):elliottt updated PR #7763.
Wasmtime GitHub notifications bot (Jan 10 2024 at 01:07):elliottt updated PR #7763.
Wasmtime GitHub notifications bot (Jan 10 2024 at 01:07):elliottt updated PR #7763.
Wasmtime GitHub notifications bot (Jan 10 2024 at 01:09):elliottt updated PR #7763.
Wasmtime GitHub notifications bot (Jan 10 2024 at 01:09):elliottt has marked PR #7763 as ready for review.
Wasmtime GitHub notifications bot (Jan 10 2024 at 01:09):elliottt requested pchickey for a review on PR #7763.
Wasmtime GitHub notifications bot (Jan 10 2024 at 01:09):elliottt requested wasmtime-core-reviewers for a review on PR #7763.
Wasmtime GitHub notifications bot (Jan 10 2024 at 01:09):elliottt requested guybedford for a review on PR #7763.
Wasmtime GitHub notifications bot (Jan 10 2024 at 01:09):elliottt requested pchickey for a review on PR #7763.
Wasmtime GitHub notifications bot (Jan 10 2024 at 04:35):pchickey submitted PR review.
Wasmtime GitHub notifications bot (Jan 10 2024 at 04:35):pchickey submitted PR review.
Wasmtime GitHub notifications bot (Jan 10 2024 at 04:35):pchickey created PR review 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?
Wasmtime GitHub notifications bot (Jan 10 2024 at 04:35):pchickey created PR review 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)
Wasmtime GitHub notifications bot (Jan 10 2024 at 04:57):elliottt submitted PR review.
Wasmtime GitHub notifications bot (Jan 10 2024 at 04:57):elliottt created PR review 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.
Wasmtime GitHub notifications bot (Jan 10 2024 at 04:57):elliottt created PR review 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.
Wasmtime GitHub notifications bot (Jan 10 2024 at 04:57):elliottt submitted PR review.
Wasmtime GitHub notifications bot (Jan 10 2024 at 18:11):elliottt submitted PR review.
Wasmtime GitHub notifications bot (Jan 10 2024 at 18:11):elliottt created PR review 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.
Wasmtime GitHub notifications bot (Jan 10 2024 at 18:59):pchickey submitted PR review.
Wasmtime GitHub notifications bot (Jan 10 2024 at 18:59):pchickey created PR review comment:
got it! sounds good.
Wasmtime GitHub notifications bot (Jan 10 2024 at 19:00):pchickey submitted PR review.
Wasmtime GitHub notifications bot (Jan 10 2024 at 19:00):pchickey created PR review comment:
works for me
Wasmtime GitHub notifications bot (Jan 10 2024 at 19:00):pchickey submitted PR review.
Wasmtime GitHub notifications bot (Jan 10 2024 at 22:10):elliottt merged PR #7763.
Last updated: Dec 23 2024 at 13:07 UTC