Chromium Code Reviews Chromium Code Reviews
Issue
887863002:
The ReadFile API on Windows invoked by the FileStream::Context class which is used by URLRequestFil… (Closed)
DescriptionThe ReadFile API on Windows invoked by the FileStream::Context class which is used by URLRequestFileJob at times completes synchronously.
The FileStream::Context class is used by the URLRequestFileJob class on the IO thread. The intention here is to open
the file for overlapped IO and thus complete the read and writes asynchonously.
Turns out that there are cases where the ReadFile API completes synchronously which ends up blocking the IO thread.
http://support.microsoft.com/kb/156932
Fix for this is to post the ReadFile call to a worker pool and inform the caller about failures etc on the calling thread.
Changes in this patch are as below:-
1. The FileStream::Context::Read function posts the ReadFile call to the Worker thread pool. The entry point for this task is the static
function ReadAsync.
This function posts results back to the originating thread via the function ReadAsyncResult
2. Fixed the FileStreamTest.WriteRead test to not read the data in the completion callback of the stream
Write function. This causes a DCHECK to fire in the MessageLoopForIO class that we are entering a nested
loop which is a no no for that message pump type.
This DCHECK fires because of the change on Windows to execute the ReadFile in a Worker thread pool.
In any case this change is safe for all platforms.
The written data is validated by reading it back in the function ValidateWrittenData in the TestWriteReadCompletionCallback class.
We call this explicitly in the FileStreamTest.WriteRead test.
3. Fix the SyncableFileOperationRunnerTest.CopyAndMove test failures.
This test internally initiates a ReadFile call on Windows via the FileStream::Context class for the Copy and Move
test. It invokes Copy and Move and then calls MessageLoop::RunUntilIdle. This call basically runs the message loop
until all pending tasks and events have been processed. This works on Windows by fluke because the ReadFile call
which completes asynchronously posts IO completion tasks to the IO thread (current thread) which then get pulled out
by the RunUntilIdle call.
There could be cases where the ReadFile call does not post IO completion events before RunUntilIdle returns and we could
see the same failures.
Reason we see this with this patch is because ReadFile is now completed by a worker pool thread which basically guarantees
the above scenario. Fix is to use base::RunLoop::Run() and Quit the loop when the DidFinish callback is received.
I changed all places in the syncable_file_operation_runner_unittest.cc file to use base::RunLoop::Run() or
base::RunLoop::RunUntilIdle() as the MessageLoop equivalents are deprecated.
BUG=423948
Committed: https://crrev.com/959eaea45546260d8517de3d1cabace790c5b5a7
Cr-Commit-Position: refs/heads/master@{#314388}
Patch Set 1 #Patch Set 2 : Fixed test failures #Patch Set 3 : Fix weakptr usage #Patch Set 4 : Ensure that the weak pointers are invalidated on Windows when the context is orphaned. #
Total comments: 4
Patch Set 5 : Fix the SyncableFileOperationRunnerTest.CopyAndMove test failures #Patch Set 6 : Address review comments #Patch Set 7 : Pass the result of ReadFile on failure only #
Total comments: 4
Messages
Total messages: 14 (4 generated)
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||
