Distributed lock implementation is not atomic

[jhadwen]

Hi,

There is a missing unique index in the lock collection on the field "Resource" (and only this field), which means that the two or more instances of a distributed lock can be created (in testing this happens frequently, especially with more than one Hangfire node).

https://docs.mongodb.com/manual/reference/method/db.collection.update/#use-unique-indexes

To add, from experience the behaviour of MongoDB is to throw a write exception if the upsert fails due to the key constraint. The standard approach is to catch the exception (returning therefore that the lock could not be acquired), see https://jira.mongodb.org/browse/SERVER-14322

Also I don't actually see any indexes being created when a new instance of the database is created. It seems indexes are only created when migrating the database?

Thanks

James

[MarLoe]

Hi @jhadwen,

Thanks for your input.

To address you the second part of your question first:
Indexes are (as you point out) created during migration. When initializing a new DB we actually run a full migration. By doing this, we do not have to maintain "two paths" for initializing the DB.
So in order to create a new index, a migration step is needed and the schema version must be increased.

In regard to the actual problem you experience:
When inserting a "distributed lock" we do use "Upsert = true".
See this line.
The options are initialized in the lines above. I thought that was enough. But perhaps you have a point that an unique index is required. I'll look into this.

/Martin

[jhadwen]

Thanks Martin,

On the index creation for a new database, I think there might be a problem then because I've never witnessed an index being created on any of the collections (using 0.5.9). Is there something specific that we need to put into MongoStorageOptions?

James

[gottscj]

@jhadwen,

This should be addressed. Can you confirm "Resource" has an index for 0.5.13 (or 0.5.12)

[jhadwen]

Hi, although I can confirm that there is now an index on "Resource" with 0.5.13, however that index is not unique which means unfortunately this issue still exists.

Thanks

James

[gottscj]

@jhadwen,

verdamt. the need for the index to be unique somehow eluded me. Looking thru the code, I see we use upsert many places without ensuring the uniqueness of the indexes.
I will update the indexes for 0.5.14

Thanks!

[jondavidford]

Hi all, I have already implemented a patch for this distributed lock issue and I will create pull request with the changes tomorrow.

[gottscj]

Fixed