Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Mutex never created on Windows. #137

Closed
schlafsack opened this Issue May 8, 2013 · 9 comments

Comments

Projects
None yet
4 participants

The following line in threading.h never actually assigns the mutex to the variable:

define NODE_SQLITE3_MUTEX_INIT CreateMutex(NULL, FALSE, NULL);

I'm also not 100% sure that the lock/unlock/destroy macros are correct. I think that pointers are being passed when instances are expected.

Could the macros be replaced with uv_mutex_* instead?

Member

springmeyer commented May 8, 2013

@schlafsack - this code was added to try to port to windows before libuv was fully developed. Using uv_mutex sounds like a great idea (now that it exists). Also, we've been using sqlite3 on windows quite a bit in this app: http://mapbox.com/tilemill/ and not hit any known threading problems. But that said, I would not be surprised if that existing unlock macros are not correct/perfect. Thanks for your feedback and any tests or patches you can provide.

I have experienced some issues with db.each.
here's a test to reproduce the issue.
on my environment (win8 x64), this test sometimes (about 1/3) fails silently.

var sqlite3 = require('sqlite3').verbose();

var db = new sqlite3.Database('test.sqlite3', function (err) {
  db.serialize(function () {
    db.run('DROP TABLE IF EXISTS t_table;');
    db.run('CREATE TABLE t_table ( name TEXT PRIMARY KEY, value TEXT );');
    for (var i = 0; i < 100; i++) {
      db.run('INSERT INTO t_table ( name, value ) VALUES ( ?, ? );', 'name' + String(i), 'value' + String(i));
    }

    console.log('performing db.each(\'SELECT * FROM t_table;\')');
    // db.each may fail with access violation
    db.each('SELECT * FROM t_table;', function (err, row) {
      ;
    }, function (err, count) {
      console.log('done');
    });
  });
});

process.on('exit', function () {
  console.log('process exited successfully');
});

and here's patch for the issue.

 src/threading.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/threading.h b/src/threading.h
index 0afa413..fe738a4 100644
--- a/src/threading.h
+++ b/src/threading.h
@@ -8,11 +8,11 @@

     #define NODE_SQLITE3_MUTEX_t HANDLE mutex;

-    #define NODE_SQLITE3_MUTEX_INIT CreateMutex(NULL, FALSE, NULL);
+    #define NODE_SQLITE3_MUTEX_INIT mutex = CreateMutex(NULL, FALSE, NULL);

-    #define NODE_SQLITE3_MUTEX_LOCK(m) WaitForSingleObject(m, INFINITE);
+    #define NODE_SQLITE3_MUTEX_LOCK(m) WaitForSingleObject(*m, INFINITE);

-    #define NODE_SQLITE3_MUTEX_UNLOCK(m) ReleaseMutex(m);
+    #define NODE_SQLITE3_MUTEX_UNLOCK(m) ReleaseMutex(*m);

     #define NODE_SQLITE3_MUTEX_DESTROY CloseHandle(mutex);
Collaborator

Mithgol commented Jun 12, 2013

@AtsushiSuzuki Could you fork node-sqlite3, edit src/threading.h and make a pull request out of your patch? There's a GitHub secret that would make the patch downloadable then (by appending .patch to your pull request's URL).

Member

springmeyer commented Jun 12, 2013

@AtsushiSuzuki - thank you very much for the report, patch, and testcase. I am planning to free up some time soon for some general node-sqlite3 development and I will take a look to see if I can replicate.

Member

springmeyer commented Jun 13, 2013

Replicated a crash/early exist here:

screen shot 2013-06-12 at 8 47 56 pm

Which is fixed by the patch.

Member

springmeyer commented Jun 13, 2013

note: for me all tests pass on windows with this patch (with the exception of two of the parallel tests that timeout - but they pass with a greater timeout value)

Member

springmeyer commented Jun 13, 2013

will now release v2.1.9 with this fix. Thanks!

Collaborator

Mithgol commented Jun 13, 2013

Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment