Skip to content

Commit

Permalink
Fix a race condition in WindowsThread (port::Thread)
Browse files Browse the repository at this point in the history
Summary:
Fix a race condition when we create a thread and immediately destroy
 This case should be supported.
  What happens is that the thread function needs the Data instance
  to actually run but has no shared ownership and must rely on the
  WindowsThread instance to continue existing.
  To address this we change unique_ptr to shared_ptr and then
  acquire an additional refcount for the threadproc which destroys it
  just before the thread exit.
  We choose to allocate shared_ptr instance on the heap as this allows
  the original thread to continue w/o waiting for the new thread to start
  running.
Closes facebook#3240

Differential Revision: D6511324

Pulled By: yiwu-arbug

fbshipit-source-id: 4633ff7996daf4d287a9fe34f60c1dd28cf4ff36
  • Loading branch information
yuslepukhin authored and facebook-github-bot committed Dec 7, 2017
1 parent 34aa245 commit fe608e3
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 6 deletions.
17 changes: 12 additions & 5 deletions port/win/win_thread.cc
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,17 @@ struct WindowsThread::Data {

void WindowsThread::Init(std::function<void()>&& func) {

data_.reset(new Data(std::move(func)));
data_ = std::make_shared<Data>(std::move(func));
// We create another instance of shared_ptr to get an additional ref
// since we may detach and destroy this instance before the threadproc
// may start to run. We choose to allocate this additional ref on the heap
// so we do not need to synchronize and allow this thread to proceed
std::unique_ptr<std::shared_ptr<Data>> th_data(new std::shared_ptr<Data>(data_));

data_->handle_ = _beginthreadex(NULL,
0, // stack size
&Data::ThreadProc,
data_.get(),
th_data.get(),
0, // init flag
&th_id_);

Expand All @@ -53,6 +58,7 @@ void WindowsThread::Init(std::function<void()>&& func) {
std::errc::resource_unavailable_try_again),
"Unable to create a thread");
}
th_data.release();
}

WindowsThread::WindowsThread() :
Expand Down Expand Up @@ -129,7 +135,7 @@ void WindowsThread::join() {
assert(false);
throw std::system_error(static_cast<int>(lastError),
std::system_category(),
"WaitForSingleObjectFailed");
"WaitForSingleObjectFailed: thread join");
}

CloseHandle(reinterpret_cast<HANDLE>(data_->handle_));
Expand Down Expand Up @@ -157,8 +163,9 @@ void WindowsThread::swap(WindowsThread& o) {
}

unsigned int __stdcall WindowsThread::Data::ThreadProc(void* arg) {
auto data = reinterpret_cast<WindowsThread::Data*>(arg);
data->func_();
auto ptr = reinterpret_cast<std::shared_ptr<Data>*>(arg);
std::unique_ptr<std::shared_ptr<Data>> data(ptr);
(*data)->func_();
_endthreadex(0);
return 0;
}
Expand Down
2 changes: 1 addition & 1 deletion port/win/win_thread.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ class WindowsThread {

struct Data;

std::unique_ptr<Data> data_;
std::shared_ptr<Data> data_;
unsigned int th_id_;

void Init(std::function<void()>&&);
Expand Down

0 comments on commit fe608e3

Please sign in to comment.