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

⛽ -> ⏲️ Switch from fuel to epoch interruptions. #273

Merged
merged 5 commits into from
Jun 2, 2023

Conversation

itsrainy
Copy link
Contributor

@itsrainy itsrainy commented May 31, 2023

This PR has two main parts:

  • Switching to epoch interruptions (the changes to lib/src/execute.rs and lib/src/linking.rs)
  • Fixing and refactoring the async_io_methods, which broke as a result of the switch.

The epoch switch is straightforward: set the config value on the wasmtime Engine and then attach a thread to the ExecuteCtx which increments the epoch every 50 µs. We also added a Drop implementation to ExecuteCtx to join that thread.

@jameysharp and I collaborated on chasing down the test issue, namely the async_io_methods test had a race condition (alluded to here) that required it to sleep for 100ms to give the backends time to work. The switch to epoch interruptions made that test flaky, so I added an additional backend and request to the test fixture as a synchronization mechanism between the guest and the server. Once the server has read enough bytes to relieve the backpressure on the write body, it will complete the sync request, signaling to the guest that the write body should be ready.

@itsrainy itsrainy force-pushed the rainy/epoch-interruptions branch from f98db2b to fde3973 Compare May 31, 2023 21:58
@itsrainy
Copy link
Contributor Author

itsrainy commented Jun 1, 2023

The test failed CI here, so I set it up to run 100 times locally, and it passed 72 times and failed 28 times. So definitely flaky. I'm still unsure why switching to epoch interruptions introduced the flakiness (or made failure more likely), but it seems like the move here is to introduce some backend synchronization mechanism to the test as described here

Copy link
Contributor

@cratelyn cratelyn left a comment

Choose a reason for hiding this comment

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

thank you for doing this ✨

@itsrainy itsrainy merged commit 362bba8 into main Jun 2, 2023
@itsrainy itsrainy deleted the rainy/epoch-interruptions branch June 2, 2023 16:17
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