I'm seeing calls to HandleProgressCallback with NULL as the pointer AsyncProgressWorker

[chris-grabcad]

I noticed that at this line it is possible for HandleProgressCallback to be called with NULL, if coming into this function asyncdata_ == NULL then it will be copied to data, and then passed into HandleProgressCallback() (the delete[] data a few lines later will pass (if it gets there, but I got an access violation before that happened :-( ).

I caught this in VS 2015 debugger by setting a conditional breakpoint where data == null

I've added some protective code in my HandleProgressCallback, but thought you might want to consider guarding against it in WorkProgress(), I think (not 100% sure) it happens just before HandleOKCallback() would be called.

Here's my HandleProgressCallback() for reference

void StlWorker::HandleProgressCallback(const char *data, size_t size) {
    Nan::HandleScope scope;

    if (m_progress && data) { // Need to check data for non-NULL (which we can be called with)
        int progress = *reinterpret_cast<int*>(const_cast<char*>(data));

        v8::Local<v8::Value> argv[] = {
            New<v8::Integer>(progress)
        };

        m_progress->Call(1, argv);
    }
}

[tossy310]

I think this is as same as #794.

[mastergberry]

Just want to throw out there this is still an issue even with the merge request from #794.

I am slamming this Send() method with maybe up to a hundred calls a second (depending how fast my 0-100% progress meter goes).

I am seeing some random nullptr's getting through still. I think with libuv there is no guarantee the data will actually be used before it is possibly deleted...based on the brief documentation here: http://docs.libuv.org/en/v1.x/async.html#c.uv_async_init

So to my understanding what is happening is something like this:

1. [Thread 0] Send() is called with some data
2. [Thread 0] SendProgress() is called internally
3. [Thread 0] async_lock is acquired
4. [Thread 0] uv_async_send(&this->async); is called which wakes up the event loop thread
5. [Thread 0] Deletes nullptr (assume there was no data waiting before and we have a fresh start) ...
6. [Thread 0] Send() is called with some new data
7. [Thread 0] SendProgress() is called internally
8. [Thread 1] Event loop thread wakes up and starts using asyncdata_ (0x1234)
9. [Thread 0] async_lock is acquired
10. [Thread 0] uv_async_send(&this->async); is called but the thread is already awake...not that it matters
11. [Thread 0] Deletes old_data (which is pointing to the info (0x1234))
12. [Thread 1] Event loop thread triggers HandleProgressCallback with nullptr (0x1234)

This is to my understanding of the code what is happening, and causing the random nullptrs...there is no safety net at the moment to ensure that Thread 1 is not having it's data deleted.

To properly fix this a thread safe queue probably needs to be implemented and the data should be deleted on Thread 1 (after the callback has been made to the user application), not on Thread 0.

[mastergberry]

I just dug around further and realized that there is AsyncProgressQueueWorker and upon inspecting the source code this is actually thread safe...since the data is deleted on the callback thread instead of the thread calling Send().

I highly recommend that the non-queue implementations are deprecated because they are not thread safe properly at the moment (unless someone wants to go and fix them).