switch_collection method is not thread safe

[matino]

Hi guys,

We just run into an issue when archiving data from one collection into another using switch_collection in multi threaded environment (sample code we used below):

def archive(self):
        self.switch_collection(Item._meta['archive_collection'])
        self.save()
        self.switch_collection(Item._meta['collection'])
        self.delete()

The scenario is a following:
1st thread runs archive, executes self.switch_collection(Item._meta['archive_collection'])
2nd thread wants to query different document and doesn't find it, because 1st thread has switched collection.

It would be a good idea to make switch_collection (and preferably switch_db) method thread safe.
What do you think?

[mmarksnippety]

Hi

I did a little investigation. Document.switch_collection internally use context manager switch_collection. That manager rewrite collection name on document class. Is this behavior necessary?

[DavidBord]

@matino , why not wrap with lock your archive and query methods?

[DavidBord]

@mmarksnippety , I think it is because otherwise how would you be able to do other actions on the second collection?

[matino]

@DavidBord - I would have to lock all queries in a non archived collection, remember about locking new queries - not really an elegant solution for me.

[DavidBord]

I think that read locking the collection while in the context of switch_collection would also lock the collection for reading from the collection switch_collection switched to

[matino]

I need to be 100% sure ;)

[DavidBord]

But its not only about you, right? :)
I don't think that starving all readers between calls to switch_collection is a proper behaviour so I am marking this as won't fix

Regardless of mongoengine, I would have separated the archive and query stuff into 2 separate processes because I wouldn't want a problem with one of them to affect the other

[matino]

@DavidBord - fine by me, thanks for your 2prompt replies anyway! Maybe there should be a warning at least in the docs about this behavior? What do you think?

[calvinwyoung]

+1 for including a warning in the docs. We've been seeing a lot of strange bugs that all turned out to be caused by the non-thread-safety of the switch_db context manager. It would've been really helpful if there were a big fat warning in the docs.

[rturk]

Why not completely review this or maybe even remove this feature? I know that this sound bald but beyond the thread issue the concept of a "switch_db" is already dangerous, doesn't looks like an streamlined code.

Ideally one should use simply use connA and connB to reference different collections, because in fact they are different connections