Windows: Remove OVERLAPPED IO flag, it is not required and seems to cause concurrency problems #2725
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Excellent! I will spin up tests tonight. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow up from #2691, apparently the current code still causes problems when concurrent reads happen in a multithreading context. I cannot figure out why, and nothing in the documentation states the way we use the Windows API is incorrect (in fact, the way we use the Windows API is almost exactly copied from Microsoft's own manual). Yet somehow the overlapped results still provide problems when used concurrently.
Removing the OVERLAPPED IO flag entirely seems to resolve the problem for me on my Windows machine. I had to pull ye olde steam powered laptop out of the attic, since the problem only seems to occur in low RAM situations. Thanks @SartorialOffense for providing an extensive example that I could use to track down this issue and fix it!
This should hopefully actually fix #2668, #2661, #2344.