my8bird / node-leveldb

NodeJS bindings to levelDB - a fast and lightweight key/value database library
http://code.google.com/p/leveldb/
BSD 2-Clause "Simplified" License
63 stars 12 forks source link

OpAsync error handler ignores errors #45

Closed justmoon closed 12 years ago

justmoon commented 12 years ago

Just noticed this while borrowing the OpAsync class for my own native binding.

Consider this code:

  template <class T> static void AsyncCallback(uv_work_t* req) {
    /* ... */

    Handle<Value> error = Null();
    Handle<Value> result = Null();

    op->Result(error, result);

    if (error.IsEmpty() && result.IsEmpty() && !op->status_.ok())
      error = Exception::Error(String::New(op->status_.ToString().c_str()));

    /* ... */
  }

The intention here seems to be: If op->Result did not set either an error nor a result and if the leveldb status is not ok, then issue an exception.

However, this doesn't work, because a handle with a value of Null() is not empty.

Handle<Value> error = Null();
error.IsEmpty() // false

Instead of IsEmpty() we want to use error.IsEmpty() || error->IsNull(). So the code above should be:

  template <class T> static void AsyncCallback(uv_work_t* req) {
    /* ... */

    Handle<Value> error = Null();
    Handle<Value> result = Null();

    op->Result(error, result);

    if ((error.IsEmpty() || error->IsNull()) && (result.IsEmpty() || result->IsNull()) &&
        !op->status_.ok())
      error = Exception::Error(String::New(op->status_.ToString().c_str()));

    /* ... */
  }

Once I fixed this problem, I got a bunch of errors from the test suite. Fortunately all of them were related to NotFound errors, so I decided to simply ignore those for the purposes of the automatic error handling.