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

fix(cli): synchronize async stdio/file reads and writes #15092

Merged
merged 16 commits into from
Jul 13, 2022

Conversation

dsherret
Copy link
Member

@dsherret dsherret commented Jul 5, 2022

Fixes a regression probably caused in #14319 where asynchronous file reads and writes no longer necessarily wrote in order.

Closes #14353

@dsherret
Copy link
Member Author

dsherret commented Jul 5, 2022

Seems like this is still flaky. I can't reproduce it as flaky on my system on Windows or Linux though https://github.com/denoland/deno/runs/7205374557?check_suite_focus=true

Edit: Oh, I reproduced it on linux after about 10 minutes of re-running the tests over and over.

@dsherret dsherret marked this pull request as draft July 5, 2022 23:37
@@ -436,6 +429,7 @@ impl StdFileResource {

async fn write(self: Rc<Self>, buf: ZeroCopyBuf) -> Result<usize, AnyError> {
let mut inner = self.inner.clone();
let _permit = self.order_semaphore.acquire().await.unwrap();
Copy link
Member Author

@dsherret dsherret Jul 6, 2022

Choose a reason for hiding this comment

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

Just verified that this is called while still executing synchronously from the JS write(...) call. Also, on the line immediately before the acquire here, the data is in order:

Hello, world! 0
Hello, world! 1
...
Hello, world! 20
Hello, world! 21
Hello, world! 22
...
Hello, world! 98
Hello, world! 99

But then after the semaphore it's (I'm not sure what xa is):

Hello, world! 0
Hello, world! 1
...
Hello, world! 20
Hello, Hello world!
Hello, world! 22
...
Hello, world! 98
Hello, world! 99
world! 21xa

Also, this always occurs at 21.

core/task_queue.rs Outdated Show resolved Hide resolved
@dsherret dsherret marked this pull request as ready for review July 6, 2022 17:04
Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

Can TaskQueue not be a runtime/ thing? I don't think it needs to be in deno_core.

runtime/ops/io.rs Outdated Show resolved Hide resolved
runtime/ops/io.rs Outdated Show resolved Hide resolved
@dsherret
Copy link
Member Author

dsherret commented Jul 12, 2022

@lucacasonato thanks for the offline feedback! I managed to switch back to an AsyncRefCell and eliminate all the mutexes being used here. I also reduced some locking on std::io::... stuff.

Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

Looks great now. If you can easially add a test for the "sync read while async read is ongoing" case, that'd be great. If it's too difficult, feel free to merge without that.

@dsherret dsherret merged commit 667812a into denoland:main Jul 13, 2022
@dsherret dsherret deleted the fix_synchronize_file_writes branch July 13, 2022 15:16
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.

stdout_write_all is flaky
3 participants