Switch to Napi from NAN #104
Conversation
| {} | ||
|
|
||
| Napi::Promise NSFW::StartWorker::RunJob() { | ||
| if (mNSFW->mInterface) { |
There was a problem hiding this comment.
This looks like it's probably undefined behavior (race condition), and should probably either be behind an std::lock_guard, wrapped in an std::atomic, or ignored and handled in Execute().
There was a problem hiding this comment.
It's not actually a race condition though, a value will either be there or it won't. If it was being destroyed while we were calling start, the operation won't start. We don't need to lock on this pointer, because we're not leveraging anything about the pointer except did it exist when we could read it at this point in time. I really don't put much emphasis on correctly queuing a new start operation when a stop operation is happening at this level.
There was a problem hiding this comment.
The correct way for a user of the library to queue a start after a stop is to wait for the stop promise to complete before running start.
There was a problem hiding this comment.
Right, but the very act of accessing a variable (in this case checking if the pointer is null) that's modified in another thread is undefined behavior unless they're atomic or synchronized. There's no guarantee that changes made in one thread will be visible in another. So it could return true, it could return false, it could never return true because maybe it cached the initial value in each thread (or put it in a register) and that thread never busted the cache, etc.
That said, if it's incorrect for users to call start while an instance is already running, I don't mind too much.
There was a problem hiding this comment.
it could never return true because maybe it cached the initial value in each thread (or put it in a register) and that thread never busted the cache, etc.
If this were ever true, why would leveraging the mutex change this behavior at all?
There was a problem hiding this comment.
I'm sorry I really don't see how this is a race condition that has any bearing on the correctness of the application. :( I specifically left out the lock here, because it will always be safe to perform this check, and will basically be correct in all cases.
There was a problem hiding this comment.
Neat. Ian shared https://devblogs.microsoft.com/oldnewthing/20180924-00/?p=99805. Under O3 optimization in G++, this can in fact force-cache the variable in the register on the thread.
| {} | ||
|
|
||
| Napi::Promise NSFW::StopWorker::RunJob() { | ||
| if (!mNSFW->mInterface) { |
There was a problem hiding this comment.
Possible race condition.
There was a problem hiding this comment.
It's really not though. We're not accessing anything on this pointer except whether it existed when we read tried to start this. We don't want to lock here. We've also handled locking this interface where we are actually doing work in the execute thread.
I've decided to move NSFW to Napi so that we can be a little more future proof in relation to the direction Node is going with native modules. This should resolve any context issues or warnings about needing to leverage newer APIs in NSFW from here on out. We might also be able to start building 1 binary per OS for Node, which would be pretty neat; though the same may not apply for Electron.
I also went ahead and upgraded any of our old dependencies and removed babel, lodash, and fs-extra from the installed dependency chain. None of those really make sense to ship here anymore.
Closes #91
Closes #96
Closes #100