diff options
author | satorux@chromium.org <satorux@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-02-29 19:29:47 +0000 |
---|---|---|
committer | satorux@chromium.org <satorux@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-02-29 19:29:47 +0000 |
commit | dbb747cc59a580ba977e5ed8feb1d74117270415 (patch) | |
tree | f7e85df659f888a64a419d0d146d3c957b946ea9 /net/base | |
parent | 62e9bad28db38512959c91005c9572ece80b3643 (diff) | |
download | chromium_src-dbb747cc59a580ba977e5ed8feb1d74117270415.zip chromium_src-dbb747cc59a580ba977e5ed8feb1d74117270415.tar.gz chromium_src-dbb747cc59a580ba977e5ed8feb1d74117270415.tar.bz2 |
net: Make destruction and closure of FileStream safer.
Before this patch, it was unsafe to destroy or close FileStream
while the file is being opened (i.e. async Open() is in flight).
BUG=115067
TEST=net_unittests
Review URL: https://chromiumcodereview.appspot.com/9467025
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@124229 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'net/base')
-rw-r--r-- | net/base/file_stream.h | 1 | ||||
-rw-r--r-- | net/base/file_stream_posix.cc | 156 | ||||
-rw-r--r-- | net/base/file_stream_posix.h | 9 | ||||
-rw-r--r-- | net/base/file_stream_unittest.cc | 54 | ||||
-rw-r--r-- | net/base/file_stream_win.cc | 104 | ||||
-rw-r--r-- | net/base/file_stream_win.h | 12 |
6 files changed, 236 insertions, 100 deletions
diff --git a/net/base/file_stream.h b/net/base/file_stream.h index a4380ec..c0db903 100644 --- a/net/base/file_stream.h +++ b/net/base/file_stream.h @@ -57,7 +57,6 @@ class NET_EXPORT FileStream { // CloseSync() does not). // // It is not OK to call Close() multiple times. The behavior is not defined. - // // Note that there must never be any pending async operations. virtual void Close(const CompletionCallback& callback); diff --git a/net/base/file_stream_posix.cc b/net/base/file_stream_posix.cc index 9c00bf3..620d35e 100644 --- a/net/base/file_stream_posix.cc +++ b/net/base/file_stream_posix.cc @@ -92,6 +92,18 @@ void OpenFile(const FilePath& path, } } +// Opens a file using OpenFile() and then signals the completion. +void OpenFileAndSignal(const FilePath& path, + int open_flags, + bool record_uma, + base::PlatformFile* file, + int* result, + base::WaitableEvent* on_io_complete, + const net::BoundNetLog& bound_net_log) { + OpenFile(path, open_flags, record_uma, file, result, bound_net_log); + on_io_complete->Signal(); +} + // Closes a file with some network logging. void CloseFile(base::PlatformFile file, const net::BoundNetLog& bound_net_log) { @@ -104,6 +116,15 @@ void CloseFile(base::PlatformFile file, bound_net_log.EndEvent(net::NetLog::TYPE_FILE_STREAM_OPEN, NULL); } +// Closes a file with CloseFile() and signals the completion. +void CloseFileAndSignal(base::PlatformFile* file, + base::WaitableEvent* on_io_complete, + const net::BoundNetLog& bound_net_log) { + CloseFile(*file, bound_net_log); + *file = base::kInvalidPlatformFileValue; + on_io_complete->Signal(); +} + // ReadFile() is a simple wrapper around read() that handles EINTR signals and // calls MapSystemError() to map errno to net error codes. void ReadFile(base::PlatformFile file, @@ -124,16 +145,16 @@ void ReadFile(base::PlatformFile file, *result = res; } -// Helper function used for FileStreamPosix::Read(). -void ReadFileFromIOBuffer(base::PlatformFile file, - scoped_refptr<IOBuffer> buf, - int buf_len, - bool record_uma, - int* result, - base::WaitableEvent* on_complete, - const net::BoundNetLog& bound_net_log) { +// Reads a file using ReadFile() and signals the completion. +void ReadFileAndSignal(base::PlatformFile file, + scoped_refptr<IOBuffer> buf, + int buf_len, + bool record_uma, + int* result, + base::WaitableEvent* on_io_complete, + const net::BoundNetLog& bound_net_log) { ReadFile(file, buf->data(), buf_len, record_uma, result, bound_net_log); - on_complete->Signal(); + on_io_complete->Signal(); } // WriteFile() is a simple wrapper around write() that handles EINTR signals and @@ -155,16 +176,16 @@ void WriteFile(base::PlatformFile file, *result = res; } -// Helper function used for FileStreamPosix::Write(). -void WriteFileToIOBuffer(base::PlatformFile file, - scoped_refptr<IOBuffer> buf, - int buf_len, - bool record_uma, - int* result, - base::WaitableEvent* on_complete, - const net::BoundNetLog& bound_net_log) { +// Writes a file using WriteFile() and signals the completion. +void WriteFileAndSignal(base::PlatformFile file, + scoped_refptr<IOBuffer> buf, + int buf_len, + bool record_uma, + int* result, + base::WaitableEvent* on_io_complete, + const net::BoundNetLog& bound_net_log) { WriteFile(file, buf->data(), buf_len, record_uma, result, bound_net_log); - on_complete->Signal(); + on_io_complete->Signal(); } // FlushFile() is a simple wrapper around fsync() that handles EINTR signals and @@ -212,16 +233,18 @@ FileStreamPosix::FileStreamPosix( FileStreamPosix::~FileStreamPosix() { if (auto_closed_) { if (open_flags_ & base::PLATFORM_FILE_ASYNC) { - // Block until the last read/write operation is complete, if needed. - // This is needed to close the file safely. - // TODO(satorux): Remove the blocking logic. - if (on_io_complete_.get()) - on_io_complete_->Wait(); - const bool posted = base::WorkerPool::PostTask( - FROM_HERE, - base::Bind(&CloseFile, file_, bound_net_log_), - true /* task_is_slow */); - DCHECK(posted); + // Block until the last open/close/read/write operation is complete. + // TODO(satorux): Ideally we should not block. crbug.com/115067 + WaitForIOCompletion(); + + // Close the file in the background. + if (IsOpen()) { + const bool posted = base::WorkerPool::PostTask( + FROM_HERE, + base::Bind(&CloseFile, file_, bound_net_log_), + true /* task_is_slow */); + DCHECK(posted); + } } else { CloseSync(); } @@ -235,28 +258,35 @@ void FileStreamPosix::Close(const CompletionCallback& callback) { DCHECK(callback_.is_null()); callback_ = callback; + + DCHECK(!on_io_complete_.get()); + on_io_complete_.reset(new base::WaitableEvent( + false /* manual_reset */, false /* initially_signaled */)); + + // Passing &file_ to a thread pool looks unsafe but it's safe here as the + // destructor ensures that the close operation is complete with + // WaitForIOCompletion(). See also the destructor. const bool posted = base::WorkerPool::PostTaskAndReply( FROM_HERE, - base::Bind(&CloseFile, file_, bound_net_log_), - base::Bind(&FileStreamPosix::OnClosed, weak_ptr_factory_.GetWeakPtr()), + base::Bind(&CloseFileAndSignal, &file_, on_io_complete_.get(), + bound_net_log_), + base::Bind(&FileStreamPosix::OnClosed, + weak_ptr_factory_.GetWeakPtr()), true /* task_is_slow */); + DCHECK(posted); } void FileStreamPosix::CloseSync() { - // Abort any existing asynchronous operations. + // TODO(satorux): Replace the following async stuff with a + // DCHECK(open_flags & ASYNC) once once all async clients are migrated to + // use Close(). crbug.com/114783 - // TODO(satorux): Replace this with a DCHECK once once all async clients - // are migrated to use Close(). crbug.com/114783 - // - // DCHECK(!(open_flags_ & base::PLATFORM_FILE_ASYNC)); + // Abort any existing asynchronous operations. weak_ptr_factory_.InvalidateWeakPtrs(); - // Block until the last read/write operation is complete, if needed. - // This is needed to close the file safely. - if (on_io_complete_.get()) { - on_io_complete_->Wait(); - on_io_complete_.reset(); // So the destructor won't be stuck. - } + // Block until the last open/read/write operation is complete. + // TODO(satorux): Ideally we should not block. crbug.com/115067 + WaitForIOCompletion(); CloseFile(file_, bound_net_log_); file_ = base::kInvalidPlatformFileValue; @@ -275,16 +305,21 @@ int FileStreamPosix::Open(const FilePath& path, int open_flags, open_flags_ = open_flags; DCHECK(open_flags_ & base::PLATFORM_FILE_ASYNC); - base::PlatformFile* file = - new base::PlatformFile(base::kInvalidPlatformFileValue); + DCHECK(!on_io_complete_.get()); + on_io_complete_.reset(new base::WaitableEvent( + false /* manual_reset */, false /* initially_signaled */)); + + // Passing &file_ to a thread pool looks unsafe but it's safe here as the + // destructor ensures that the open operation is complete with + // WaitForIOCompletion(). See also the destructor. int* result = new int(OK); const bool posted = base::WorkerPool::PostTaskAndReply( FROM_HERE, - base::Bind(&OpenFile, path, open_flags, record_uma_, file, result, - bound_net_log_), - base::Bind(&FileStreamPosix::OnOpened, + base::Bind(&OpenFileAndSignal, + path, open_flags, record_uma_, &file_, result, + on_io_complete_.get(), bound_net_log_), + base::Bind(&FileStreamPosix::OnIOComplete, weak_ptr_factory_.GetWeakPtr(), - base::Owned(file), base::Owned(result)), true /* task_is_slow */); DCHECK(posted); @@ -378,7 +413,7 @@ int FileStreamPosix::Read( false /* manual_reset */, false /* initially_signaled */)); const bool posted = base::WorkerPool::PostTaskAndReply( FROM_HERE, - base::Bind(&ReadFileFromIOBuffer, file_, buf, buf_len, + base::Bind(&ReadFileAndSignal, file_, buf, buf_len, record_uma_, result, on_io_complete_.get(), bound_net_log_), base::Bind(&FileStreamPosix::OnIOComplete, weak_ptr_factory_.GetWeakPtr(), @@ -443,7 +478,7 @@ int FileStreamPosix::Write( false /* manual_reset */, false /* initially_signaled */)); const bool posted = base::WorkerPool::PostTaskAndReply( FROM_HERE, - base::Bind(&WriteFileToIOBuffer, file_, buf, buf_len, + base::Bind(&WriteFileAndSignal, file_, buf, buf_len, record_uma_, result, on_io_complete_.get(), bound_net_log_), base::Bind(&FileStreamPosix::OnIOComplete, weak_ptr_factory_.GetWeakPtr(), @@ -534,27 +569,24 @@ base::PlatformFile FileStreamPosix::GetPlatformFileForTesting() { void FileStreamPosix::OnClosed() { file_ = base::kInvalidPlatformFileValue; - - CompletionCallback temp = callback_; - callback_.Reset(); - temp.Run(OK); -} - -void FileStreamPosix::OnOpened(base::PlatformFile* file, int* result) { - file_ = *file; - - CompletionCallback temp = callback_; - callback_.Reset(); - temp.Run(*result); + int result = OK; + OnIOComplete(&result); } void FileStreamPosix::OnIOComplete(int* result) { CompletionCallback temp_callback = callback_; callback_.Reset(); - // Reset this earlier than Run() as Run() may end up calling Read/Write(). + // Reset this before Run() as Run() may issue a new async operation. on_io_complete_.reset(); temp_callback.Run(*result); } +void FileStreamPosix::WaitForIOCompletion() { + if (on_io_complete_.get()) { + on_io_complete_->Wait(); + on_io_complete_.reset(); + } +} + } // namespace net diff --git a/net/base/file_stream_posix.h b/net/base/file_stream_posix.h index 674bd23..a8b919f 100644 --- a/net/base/file_stream_posix.h +++ b/net/base/file_stream_posix.h @@ -51,11 +51,6 @@ class NET_EXPORT FileStreamPosix { base::PlatformFile GetPlatformFileForTesting(); private: - // Called when the file_ is opened asynchronously. |file| contains the - // platform file opened. |result| contains the result as a network error - // code. - void OnOpened(base::PlatformFile *file, int* result); - // Called when the file_ is closed asynchronously. void OnClosed(); @@ -63,6 +58,10 @@ class NET_EXPORT FileStreamPosix { // |result| contains the result as a network error code. void OnIOComplete(int* result); + // Waits until the in-flight async open/close/read/write operation is + // complete. + void WaitForIOCompletion(); + base::PlatformFile file_; int open_flags_; bool auto_closed_; diff --git a/net/base/file_stream_unittest.cc b/net/base/file_stream_unittest.cc index 7360c54..39719e2d 100644 --- a/net/base/file_stream_unittest.cc +++ b/net/base/file_stream_unittest.cc @@ -1124,6 +1124,60 @@ TEST_F(FileStreamTest, AsyncWriteAndCloseSync) { stream.CloseSync(); } +// TODO(satorux): This should be gone once all once all async clients are +// migrated to use Close(). crbug.com/114783 +TEST_F(FileStreamTest, AsyncOpenAndCloseSync) { + FileStream stream(NULL); + int flags = base::PLATFORM_FILE_OPEN | + base::PLATFORM_FILE_WRITE | + base::PLATFORM_FILE_ASYNC; + TestCompletionCallback open_callback; + int rv = stream.Open(temp_file_path(), flags, open_callback.callback()); + EXPECT_EQ(ERR_IO_PENDING, rv); + + // Close the stream without waiting for the completion. Should be safe. + stream.CloseSync(); + // open_callback won't be called. + EXPECT_FALSE(open_callback.have_result()); +} + +TEST_F(FileStreamTest, AsyncOpenAndDelete) { + scoped_ptr<FileStream> stream(new FileStream(NULL)); + int flags = base::PLATFORM_FILE_OPEN | + base::PLATFORM_FILE_WRITE | + base::PLATFORM_FILE_ASYNC; + TestCompletionCallback open_callback; + int rv = stream->Open(temp_file_path(), flags, open_callback.callback()); + EXPECT_EQ(ERR_IO_PENDING, rv); + + // Delete the stream without waiting for the open operation to be + // complete. Should be safe. + stream.reset(); + // open_callback won't be called. + EXPECT_FALSE(open_callback.have_result()); +} + +TEST_F(FileStreamTest, AsyncCloseAndDelete) { + scoped_ptr<FileStream> stream(new FileStream(NULL)); + int flags = base::PLATFORM_FILE_OPEN | + base::PLATFORM_FILE_WRITE | + base::PLATFORM_FILE_ASYNC; + TestCompletionCallback open_callback; + int rv = stream->Open(temp_file_path(), flags, open_callback.callback()); + EXPECT_EQ(ERR_IO_PENDING, rv); + EXPECT_EQ(OK, open_callback.WaitForResult()); + EXPECT_TRUE(stream->IsOpen()); + + TestCompletionCallback close_callback; + stream->Close(close_callback.callback()); + + // Delete the stream without waiting for the close operation to be + // complete. Should be safe. + stream.reset(); + // close_callback won't be called. + EXPECT_FALSE(close_callback.have_result()); +} + } // namespace } // namespace net diff --git a/net/base/file_stream_win.cc b/net/base/file_stream_win.cc index 732919e..e37ae30 100644 --- a/net/base/file_stream_win.cc +++ b/net/base/file_stream_win.cc @@ -62,9 +62,9 @@ int RecordAndMapError(int error, void OpenFile(const FilePath& path, int open_flags, bool record_uma, - const net::BoundNetLog& bound_net_log, base::PlatformFile* file, - int* result) { + int* result, + const net::BoundNetLog& bound_net_log) { bound_net_log.BeginEvent( net::NetLog::TYPE_FILE_STREAM_OPEN, make_scoped_refptr( @@ -84,6 +84,18 @@ void OpenFile(const FilePath& path, } } +// Opens a file using OpenFile() and signals the completion. +void OpenFileAndSignal(const FilePath& path, + int open_flags, + bool record_uma, + base::PlatformFile* file, + int* result, + base::WaitableEvent* on_io_complete, + const net::BoundNetLog& bound_net_log) { + OpenFile(path, open_flags, record_uma, file, result, bound_net_log); + on_io_complete->Signal(); +} + // Closes a file with some network logging. void CloseFile(base::PlatformFile file, const net::BoundNetLog& bound_net_log) { @@ -98,6 +110,15 @@ void CloseFile(base::PlatformFile file, bound_net_log.EndEvent(net::NetLog::TYPE_FILE_STREAM_OPEN, NULL); } +// Closes a file with CloseFile() and signals the completion. +void CloseFileAndSignal(base::PlatformFile* file, + base::WaitableEvent* on_io_complete, + const net::BoundNetLog& bound_net_log) { + CloseFile(*file, bound_net_log); + *file = base::kInvalidPlatformFileValue; + on_io_complete->Signal(); +} + } // namespace // FileStreamWin::AsyncContext ---------------------------------------------- @@ -222,18 +243,21 @@ FileStreamWin::FileStreamWin( FileStreamWin::~FileStreamWin() { if (auto_closed_) { - if (async_context_.get()) { - // Block until the last read/write operation is complete, if needed. - // This is needed to close the file safely. - // TODO(satorux): Remove the blocking logic. crrev.com/115067 + if (open_flags_ & base::PLATFORM_FILE_ASYNC) { + // Block until the in-flight open/close operation is complete. + // TODO(satorux): Ideally we should not block. crbug.com/115067 + WaitForIOCompletion(); + // Block until the last read/write operation is complete. async_context_.reset(); // Close the file in the background. - const bool posted = base::WorkerPool::PostTask( - FROM_HERE, - base::Bind(&CloseFile, file_, bound_net_log_), - true /* task_is_slow */); - DCHECK(posted); + if (IsOpen()) { + const bool posted = base::WorkerPool::PostTask( + FROM_HERE, + base::Bind(&CloseFile, file_, bound_net_log_), + true /* task_is_slow */); + DCHECK(posted); + } } else { CloseSync(); } @@ -246,14 +270,19 @@ void FileStreamWin::Close(const CompletionCallback& callback) { DCHECK(callback_.is_null()); callback_ = callback; - // Make sure we don't have a request in flight. Unlike CloseSync(), don't - // abort existing asynchronous operations, as it'd block. - DCHECK(async_context_.get()); - DCHECK(async_context_->callback().is_null()); + DCHECK(open_flags_ & base::PLATFORM_FILE_ASYNC); + + DCHECK(!on_io_complete_.get()); + on_io_complete_.reset(new base::WaitableEvent( + false /* manual_reset */, false /* initially_signaled */)); + // Passing &file_ to a thread pool looks unsafe but it's safe here as the + // destructor ensures that the close operation is complete with + // WaitForIOCompletion(). See also the destructor. const bool posted = base::WorkerPool::PostTaskAndReply( FROM_HERE, - base::Bind(&CloseFile, file_, bound_net_log_), + base::Bind(&CloseFileAndSignal, &file_, on_io_complete_.get(), + bound_net_log_), base::Bind(&FileStreamWin::OnClosed, weak_ptr_factory_.GetWeakPtr()), true /* task_is_slow */); DCHECK(posted); @@ -267,8 +296,10 @@ void FileStreamWin::CloseSync() { if (file_ != base::kInvalidPlatformFileValue) CancelIo(file_); - // TODO(satorux): Remove this once all async clients are migrated to use - // Close(). crbug.com/114783 + // TODO(satorux): Replace this with a DCHECK(open_flags & ASYNC) once this + // once all async clients are migrated to use Close(). crbug.com/114783 + WaitForIOCompletion(); + // Block until the last read/write operation is complete. async_context_.reset(); if (file_ != base::kInvalidPlatformFileValue) { @@ -281,7 +312,7 @@ void FileStreamWin::CloseSync() { } int FileStreamWin::Open(const FilePath& path, int open_flags, - const CompletionCallback& callback) { + const CompletionCallback& callback) { if (IsOpen()) { DLOG(FATAL) << "File is already open!"; return ERR_UNEXPECTED; @@ -293,16 +324,21 @@ int FileStreamWin::Open(const FilePath& path, int open_flags, open_flags_ = open_flags; DCHECK(open_flags_ & base::PLATFORM_FILE_ASYNC); - base::PlatformFile* file = - new base::PlatformFile(base::kInvalidPlatformFileValue); + DCHECK(!on_io_complete_.get()); + on_io_complete_.reset(new base::WaitableEvent( + false /* manual_reset */, false /* initially_signaled */)); + + // Passing &file_ to a thread pool looks unsafe but it's safe here as the + // destructor ensures that the open operation is complete with + // WaitForIOCompletion(). See also the destructor. int* result = new int(OK); const bool posted = base::WorkerPool::PostTaskAndReply( FROM_HERE, - base::Bind(&OpenFile, path, open_flags, record_uma_, bound_net_log_, - file, result), + base::Bind(&OpenFileAndSignal, + path, open_flags, record_uma_, &file_, result, + on_io_complete_.get(), bound_net_log_), base::Bind(&FileStreamWin::OnOpened, weak_ptr_factory_.GetWeakPtr(), - base::Owned(file), base::Owned(result)), true /* task_is_slow */); DCHECK(posted); @@ -318,7 +354,7 @@ int FileStreamWin::OpenSync(const FilePath& path, int open_flags) { open_flags_ = open_flags; int result = OK; - OpenFile(path, open_flags_, record_uma_, bound_net_log_, &file_, &result); + OpenFile(path, open_flags_, record_uma_, &file_, &result, bound_net_log_); if (result != OK) return result; @@ -626,12 +662,14 @@ void FileStreamWin::OnClosed() { CompletionCallback temp = callback_; callback_.Reset(); + + // Reset this before Run(). Run() should not issue a new async operation + // here, but just to keep it consistent with OnOpened(). + on_io_complete_.reset(); temp.Run(OK); } -void FileStreamWin::OnOpened(base::PlatformFile* file, int* result) { - file_ = *file; - +void FileStreamWin::OnOpened(int* result) { if (*result == OK) { async_context_.reset(new AsyncContext(bound_net_log_)); if (record_uma_) @@ -642,7 +680,17 @@ void FileStreamWin::OnOpened(base::PlatformFile* file, int* result) { CompletionCallback temp = callback_; callback_.Reset(); + + // Reset this before Run() as Run() may issue a new async operation. + on_io_complete_.reset(); temp.Run(*result); } +void FileStreamWin::WaitForIOCompletion() { + if (on_io_complete_.get()) { + on_io_complete_->Wait(); + on_io_complete_.reset(); + } +} + } // namespace net diff --git a/net/base/file_stream_win.h b/net/base/file_stream_win.h index cc6f565..5b4dd7e 100644 --- a/net/base/file_stream_win.h +++ b/net/base/file_stream_win.h @@ -11,6 +11,7 @@ #include "base/memory/scoped_ptr.h" #include "base/memory/weak_ptr.h" #include "base/platform_file.h" +#include "base/synchronization/waitable_event.h" #include "net/base/completion_callback.h" #include "net/base/file_stream_whence.h" #include "net/base/net_export.h" @@ -52,14 +53,16 @@ class NET_EXPORT FileStreamWin { class AsyncContext; friend class AsyncContext; - // Called when the file_ is opened asynchronously. |file| contains the - // platform file opened. |result| contains the result as a network error - // code. - void OnOpened(base::PlatformFile *file, int* result); + // Called when the file_ is opened asynchronously. |result| contains the + // result as a network error code. + void OnOpened(int* result); // Called when the file_ is closed asynchronously. void OnClosed(); + // Waits until the in-flight async open/close operation is complete. + void WaitForIOCompletion(); + // This member is used to support asynchronous reads. It is non-null when // the FileStreamWin was opened with PLATFORM_FILE_ASYNC. scoped_ptr<AsyncContext> async_context_; @@ -71,6 +74,7 @@ class NET_EXPORT FileStreamWin { net::BoundNetLog bound_net_log_; base::WeakPtrFactory<FileStreamWin> weak_ptr_factory_; CompletionCallback callback_; + scoped_ptr<base::WaitableEvent> on_io_complete_; DISALLOW_COPY_AND_ASSIGN(FileStreamWin); }; |