SequelizeTimeoutError: SQLITE_BUSY: database is locked

[Eric Thompson]

I am writing an application that will run into competing database locks when using SQLite.  This causes an error: "SequelizeTimeoutError: SQLITE_BUSY: database is locked".  In databases other than SQLite I would be able to do a read for lock and avoid the busy state, but SQLite kind punts on the entire thing and throws BUSY errors instead of waiting for the database lock to go away.  The only way to resolve this seems to be adding a re-try loop when the database reports busy.  I don't want to implement that each and every call I make to the database from my application code.  So that brings me to:

Would a "retry" option be a legitimate thing for Sequelize to implement?  I'm thinking "retry: 5" would retry the call 5 times if the error code coming back is "SQLITE_BUSY", with a simple step-back function so its not hammering the busy database.  Once the retry count had been hit it would still throw the busy error.  Any other error and it would immediately throw the error like normal.

Thoughts?

[Mick Hansen]

We've previously had a feature request for retrying stuff like transactions.
I would suggest adding a match or similar feature to https://github.com/mickhansen/retry-as-promised and then integration retry-as-promised into queries and transactions somehow.

--

Mick Hansen

@mhansendev
mhansen.io

[Eric Thompson]

So that is something Sequelize would be interested in supporting?  We would be happy to tackle it.

[Mick Hansen]

Most likely yes :)

[Eric Thompson]

Cool.  Would you prefer your retry-as-promised project be used, or just re-implemented in the Sequelize code?

[Mick Hansen]

If it can be solved by a well tested library rather than us having to maintain an extra component that would be advantageous :)

[Eric Thompson]

I'm thinking I can wrap this promise in your retry project... but I'm not quite sure if it would be better to put it in the nested promise here (run).

Your guidance is appreciated.

[Eric Thompson]

After some testing, the best place to put it seems to be the second option.  What we have now looks like this:

    var query = new self.dialect.Query(connection, self, options);

    return retry(function() {

        return query.run(sql, bindParameters).finally(function() {

        if (options.transaction) return;

        return self.connectionManager.releaseConnection(connection);

      });

    }, {

      max: 15  // will turn into variable

    });

I would like to further wrap that in some "only-retry-if" code so we only retry on BUSY error state.  Do you think the best idea would be to add a callback to the retry() function that can do the retry-or-not checking?

[Mick Hansen]

We should probably have retries both on transaction and query level, but let it be configured by the user for now.
Implement `match` functionality in retry-as-promised which only retries if error is instanceof match or message matches string.

In Sequelize we should add a retry integer and match option that maps to retry-as-promised. retry-as-promised with max: 1 should effectively be no-op AFAIK.

[Eric Thompson]

This handles both transactions and queries (the transaction uses this raw query function).

I will fork and implement match in retry-as-promised. 

Agreed on the rest of the behavior. 

[Mick Hansen]

If a transaction fail you might need to retry the entire transaction. Atleast in postgres if any query errors the entire transaction is void.

[Eric Thompson]

It seems like that should still be left to the application code to handle.  I doubt it's logically safe to retry under those conditions.  Even if it were, it would require Sequelize to maintain a stack of calls that the transaction has made already and do some very complicated logic on retry...

[Mick Hansen]

Not really, the transaction would automatically rollback and then retry from the beginning - It would only work for the managed transactions version though, where we have a callback to retry.
Would mostly be for handling cases of deadlock under high concurrency locking conditions.
I atleast recall there being a feature request somewhere :)

[Eric Thompson]

So, just to make sure I understand what you are suggesting... while inside a transaction, many calls could be made like so:

sequelize.transaction().then(function(t) {

  models.Task.find({where: {...}, transaction: t}).then(function(tasks) {

    var updates = [];

    tasks.forEach(function(task) {

      if(<some logic>)

        updates.push(task.update({<whatever>}))

    });

    all(updates).then(function() {

      t.commit();

    });

  }

}

You are saying that if any of those find or update calls fail (with deadlock or busy), then it should re-try the logic?

[Mick Hansen]

Not for that syntax, it should be opt-in for users on the managed syntax:

sequelize.transaction(function (t) {
  return doStuffWithTransaction({transaction: t});
}, {
  retry: {
    max: 3,
    match: 'DEADLOCK' // or something
  }
});

[Eric Thompson]

Ok, I see what you are saying now.  Will look into it after making upgrades to retry-as-promised.

[Mick Hansen]

Great :) Not a necessarily a priority just a nice to have.

[Eric Thompson]

K, I have a lot to do and if it looks like more than a half day's work I might leave it then - but I will look at it.