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

src: use RAII for mutexes and condition variables #7334

Merged
merged 0 commits into from Jun 21, 2016

Conversation

Projects
None yet
7 participants
@bnoordhuis
Member

bnoordhuis commented Jun 18, 2016

We will be introducing many more critical sections in the upcoming
multi-isolate changes, so let's make manual synchronization a thing
of the past.

CI: https://ci.nodejs.org/job/node-test-pull-request/3022/

@bnoordhuis

View changes

src/inspector_agent.cc Outdated
@@ -470,21 +462,19 @@ void AgentImpl::OnRemoteDataIO(uv_stream_t* stream,
}
DisconnectAndDisposeIO(socket);
}
uv_cond_broadcast(&agent->pause_cond_);
agent->pause_cond_.Broadcast(scoped_lock);

This comment has been minimized.

@bnoordhuis

bnoordhuis Jun 18, 2016

Member

@pavelfeldman Can you comment on whether the changes to this function look correct to you? ISTM it should be holding the lock when signalling the condition variable.

@addaleax

This comment has been minimized.

Member

addaleax commented Jun 18, 2016

@bnoordhuis The next time you rebase this against master, could you include addaleax/node@d16e607? Sorry it took me so long to get #6635 landed.

@bnoordhuis bnoordhuis force-pushed the bnoordhuis:raii-lock branch Jun 18, 2016

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Jun 18, 2016

Rebased, with workaround for compilers that don't understand constexpr function pointers: https://ci.nodejs.org/job/node-test-pull-request/3024/

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Jun 19, 2016

Green except for flaky test parallel/test-vm-timeout, tracked in #6727.

@jasnell

This comment has been minimized.

Member

jasnell commented Jun 20, 2016

nice. LGTM

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented Jun 20, 2016

LGTM

@trevnorris

View changes

src/node_mutex.h Outdated
inline ~ConditionVariableBase();
inline void Broadcast(const ScopedLock&);
inline void Signal(const ScopedLock&);
inline void Wait(const ScopedLock& scoped_lock);

This comment has been minimized.

@trevnorris

trevnorris Jun 20, 2016

Contributor

reason for adding the argument name scoped_lock in Wait() but not in Broadcast() or Signal()?

This comment has been minimized.

@bnoordhuis

bnoordhuis Jun 20, 2016

Member

Wait() uses it, the other two just take it to prove ownership (i.e., you can't signal or broadcast if you don't hold the lock.)

@trevnorris

View changes

src/node_mutex.h Outdated
struct LibuvMutexTraits {
using CondT = uv_cond_t;
using MutexT = uv_mutex_t;

This comment has been minimized.

@trevnorris

trevnorris Jun 20, 2016

Contributor

mind sharing why you did it this way? i can't figure out why you would have done it this way for a struct.

This comment has been minimized.

@bnoordhuis

bnoordhuis Jun 20, 2016

Member

You mean using instead of typedef or why a traits struct in the first place? using because it's shorter and C++11-y, a traits struct because I like the flexibility; if the day comes we need a pthreads-specific implementation, or a spinlock, or even a no-op lock, MutexBase is ready.

@trevnorris

View changes

src/node_mutex.h Outdated
}
template <typename Traits>
MutexBase<Traits>::ScopedLock::ScopedLock(const MutexBase& mutex)

This comment has been minimized.

@trevnorris

trevnorris Jun 20, 2016

Contributor

would it be sane, or even possible, to add a static_assert that checks if the class is stack allocated?

This comment has been minimized.

@bnoordhuis

bnoordhuis Jun 20, 2016

Member

If I understand you right, that's enforced by the DISALLOW_COPY_AND_ASSIGN macros in the class declarations.

EDIT: Or are you referring to disallowing heap allocations? Why would you want to?

This comment has been minimized.

@trevnorris

trevnorris Jun 20, 2016

Contributor

Or are you referring to disallowing heap allocations? Why would you want to?

Mainly because I don't see the need for a user to do new Mutex::ScopedLock(mutex). Which would seem to defeat the purpose. It was just a fleeting thought.

This comment has been minimized.

@bnoordhuis

bnoordhuis Jun 21, 2016

Member

I can add code that prohibits directly heap-allocating it (see below) but that can still be subverted by using ::new Mutex::ScopedLock(...) or by embedding the ScopedLock in a heap-allocated struct.

If you still think it's a good idea, LMK and I'll follow up with a pull request.

diff --git a/src/node_mutex.h b/src/node_mutex.h
index 9e1d316..e14735a 100644
--- a/src/node_mutex.h
+++ b/src/node_mutex.h
@@ -35,6 +35,7 @@ class MutexBase {
     friend class ScopedUnlock;
     const MutexBase& mutex_;
     DISALLOW_COPY_AND_ASSIGN(ScopedLock);
+    DISALLOW_NEW_AND_DELETE();
   };

   class ScopedUnlock {
@@ -46,6 +47,7 @@ class MutexBase {
     friend class ScopedLock;
     const MutexBase& mutex_;
     DISALLOW_COPY_AND_ASSIGN(ScopedUnlock);
+    DISALLOW_NEW_AND_DELETE();
   };

  private:
diff --git a/src/util.h b/src/util.h
index 9ad0f6c..a3e211a 100644
--- a/src/util.h
+++ b/src/util.h
@@ -33,6 +33,13 @@ template <typename T> using remove_reference = std::remove_reference<T>;
   TypeName(const TypeName&) = delete;                                         \
   TypeName(TypeName&&) = delete

+#define DISALLOW_NEW_AND_DELETE()                                             \
+  void* operator new(size_t) = delete;                                        \
+  void* operator new[](size_t) = delete;                                      \
+  void operator delete(void*, size_t) = delete;                               \
+  void operator delete[](void*, size_t) = delete
+
 // Windows 8+ does not like abort() in Release mode
 #ifdef _WIN32
 #define ABORT() raise(SIGABRT)
@trevnorris

This comment has been minimized.

Contributor

trevnorris commented Jun 20, 2016

Thanks for answering the questions. Makes sense. LGTM.

@bnoordhuis bnoordhuis closed this Jun 21, 2016

@bnoordhuis bnoordhuis force-pushed the bnoordhuis:raii-lock branch to 781713d Jun 21, 2016

bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Jun 21, 2016

tools: disable readability/function cpplint rule
cpplint gets too easily confused by C++ constructs that look like
function declarations but aren't.  Furthermore, it's arguably a
bad rule that conflicts with gcc's -Wunused-parameter flag.

PR-URL: nodejs#7334
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>

bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Jun 21, 2016

src: use RAII for mutexes and condition variables
We will be introducing many more critical sections in the upcoming
multi-isolate changes, so let's make manual synchronization a thing
of the past.

PR-URL: nodejs#7334
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>

@bnoordhuis bnoordhuis deleted the bnoordhuis:raii-lock branch Jun 21, 2016

@bnoordhuis bnoordhuis merged commit 781713d into nodejs:master Jun 21, 2016

evanlucas added a commit that referenced this pull request Jun 27, 2016

tools: disable readability/function cpplint rule
cpplint gets too easily confused by C++ constructs that look like
function declarations but aren't.  Furthermore, it's arguably a
bad rule that conflicts with gcc's -Wunused-parameter flag.

PR-URL: #7334
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented Jun 27, 2016

This touches the inspector. Marking as dont-land-on-v6. (for now, idk until we resolve that)

@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented Jul 5, 2016

@bnoordhuis 781713d applies poorly on v6 but as far as I can tell the other commit is required to not have more conflicts on the inspector...

If possible, separating out non-backportable work would be greatly appreciated by us releasers!

Fishrock123 added a commit that referenced this pull request Jul 5, 2016

src: use RAII for mutexes and condition variables
We will be introducing many more critical sections in the upcoming
multi-isolate changes, so let's make manual synchronization a thing
of the past.

PR-URL: #7334
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>

@Fishrock123 Fishrock123 referenced this pull request Jul 5, 2016

Merged

Propose v6.3.0 (v2) #7550

@MylesBorins

This comment has been minimized.

Member

MylesBorins commented Jul 11, 2016

@bnoordhuis should this be backported?

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Jul 12, 2016

@thealphanerd It's not necessary per se but it would be good. You'll have to drop the changes to src/inspector_agent.cc.

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Jul 13, 2016

#7715 - v4.x back-port

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented Aug 10, 2016

@bnoordhuis this doesn't seem to apply cleanly to v6.x either, and #7715 has 213 commits. Could you backport this?

bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Sep 5, 2016

src: use RAII for mutexes and condition variables
We will be introducing many more critical sections in the upcoming
multi-isolate changes, so let's make manual synchronization a thing
of the past.

PR-URL: nodejs#7334
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Sep 5, 2016

For posterity, this landed in v6.x in commit 0593351.

MylesBorins added a commit that referenced this pull request Sep 7, 2016

src: use RAII for mutexes and condition variables
We will be introducing many more critical sections in the upcoming
multi-isolate changes, so let's make manual synchronization a thing
of the past.

PR-URL: #7334
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>

MylesBorins added a commit that referenced this pull request Sep 28, 2016

src: use RAII for mutexes and condition variables
We will be introducing many more critical sections in the upcoming
multi-isolate changes, so let's make manual synchronization a thing
of the past.

PR-URL: #7334
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>

rvagg added a commit that referenced this pull request Oct 18, 2016

src: use RAII for mutexes and condition variables
We will be introducing many more critical sections in the upcoming
multi-isolate changes, so let's make manual synchronization a thing
of the past.

PR-URL: #7334
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>

MylesBorins added a commit that referenced this pull request Oct 26, 2016

src: use RAII for mutexes and condition variables
We will be introducing many more critical sections in the upcoming
multi-isolate changes, so let's make manual synchronization a thing
of the past.

PR-URL: #7334
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>

@MylesBorins MylesBorins referenced this pull request Oct 26, 2016

Closed

V4.6.2 proposal #9298

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment