Skip to content

Commit

Permalink
src: track no of active JS signal handlers
Browse files Browse the repository at this point in the history
This makes it possible to tell whether a signal is being tracked in JS.

PR-URL: #30229
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
  • Loading branch information
addaleax committed Nov 6, 2019
1 parent 4aca277 commit 55f98df
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 0 deletions.
2 changes: 2 additions & 0 deletions src/node_internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,8 @@ void EndStartedProfilers(Environment* env);
}
#endif // HAVE_INSPECTOR

bool HasSignalJSHandler(int signum);

#ifdef _WIN32
typedef SYSTEMTIME TIME_TYPE;
#else // UNIX, OSX
Expand Down
38 changes: 38 additions & 0 deletions src/signal_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,13 @@ using v8::Object;
using v8::String;
using v8::Value;

void DecreaseSignalHandlerCount(int signum);

namespace {

static Mutex handled_signals_mutex;
static std::map<int, int64_t> handled_signals; // Signal -> number of handlers

class SignalWrap : public HandleWrap {
public:
static void Initialize(Local<Object> target,
Expand Down Expand Up @@ -85,6 +90,11 @@ class SignalWrap : public HandleWrap {
CHECK_EQ(r, 0);
}

void Close(v8::Local<v8::Value> close_callback) override {
if (active_) DecreaseSignalHandlerCount(handle_.signum);
HandleWrap::Close(close_callback);
}

static void Start(const FunctionCallbackInfo<Value>& args) {
SignalWrap* wrap;
ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());

This comment has been minimized.

Copy link
@yorkie

yorkie Nov 22, 2019

Contributor

Should we check the wrap->handle_.signum == signum here? This might cause an incorrect signal count when calling this method in multiple times. And I also think it's still a work around for uv's uv_signal_start which doesn't return a specific result for just changing the callback.

This comment has been minimized.

Copy link
@addaleax

addaleax Nov 22, 2019

Author Member

wrap->handle_.signum is not set here yet, only after uv_signal_start().

That this method isn’t called multiple times (successfully) should be verified by the CHECK(!wrap->active_); below?

This comment has been minimized.

Copy link
@yorkie

yorkie Nov 22, 2019

Contributor

@addaleax you are right, CHECK(!wrap->active_) looks avoid this, thanks :)

This comment has been minimized.

Copy link
@yorkie

yorkie Nov 22, 2019

Contributor

Hi, I have another question, calling start() with different signal number in times, and the non-first calls might be also prevented by this CHECK?

This comment has been minimized.

Copy link
@yorkie

yorkie Nov 22, 2019

Contributor

How about exposing a method at libuv like uv_signal_has_handler(signum), because the internal uv__signal_tree_s seems to be static globally and used to store all signal handles.

This comment has been minimized.

Copy link
@addaleax

addaleax Nov 22, 2019

Author Member

Hi, I have another question, calling start() with different signal number in times, and the non-first calls might be also prevented by this CHECK?

That would be a bug in Node.js then, right? Calling .start() multiple times?

How about exposing a method at libuv like uv_signal_has_handler(signum), because the internal uv__signal_tree_s seems to be static globally and used to store all signal handles.

I think that would be okay, yes? Feel free to work on a PR if you like, the only issue I could see is that it’s hard to use that API in a way that doesn’t lead to race conditions in multi-threaded environments (but then again what we do here isn’t perfect in that regard either)

Expand Down Expand Up @@ -112,21 +122,49 @@ class SignalWrap : public HandleWrap {
wrap->MakeCallback(env->onsignal_string(), 1, &arg);
},
signum);

if (err == 0) {
CHECK(!wrap->active_);
wrap->active_ = true;
Mutex::ScopedLock lock(handled_signals_mutex);
handled_signals[signum]++;
}

args.GetReturnValue().Set(err);
}

static void Stop(const FunctionCallbackInfo<Value>& args) {
SignalWrap* wrap;
ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());

if (wrap->active_) {
wrap->active_ = false;
DecreaseSignalHandlerCount(wrap->handle_.signum);
}

int err = uv_signal_stop(&wrap->handle_);
args.GetReturnValue().Set(err);
}

uv_signal_t handle_;
bool active_ = false;
};


} // anonymous namespace

void DecreaseSignalHandlerCount(int signum) {
Mutex::ScopedLock lock(handled_signals_mutex);
int new_handler_count = --handled_signals[signum];
CHECK_GE(new_handler_count, 0);
if (new_handler_count == 0)
handled_signals.erase(signum);
}

bool HasSignalJSHandler(int signum) {
Mutex::ScopedLock lock(handled_signals_mutex);
return handled_signals.find(signum) != handled_signals.end();
}
} // namespace node


Expand Down

0 comments on commit 55f98df

Please sign in to comment.