MorphiaOrg / morphia

MongoDB object-document mapper in Java based on https://github.com/mongodb/mongo-java-driver
Apache License 2.0
1.64k stars 455 forks source link

dev.morphia.Datastore Thread Safety #3035

Open MKaramavrov opened 3 months ago

MKaramavrov commented 3 months ago

Discussed in https://github.com/MorphiaOrg/morphia/discussions/2802

Originally posted by **AmitKuGarg** December 18, 2023 **Describe the bug** When dev.morphia.Datastore shared across thread, I get Two entities have been mapped using the same discriminator value. **To Reproduce** Steps to reproduce the behavior: 1. Create a sample collection and create the datastore. Datastore datastore = Morphia.createDatastore(mongo, dbName); datastore.ensureIndexes(); if(this.packages != null){ for (String packageString : packages) { datastore.getMapper().map(packageString); } } 2. Create a runnable class and pass the data store and id parameter. In run method load the data from the collection using id and datastore. class Task implements Runnable { private String id; private Datastore mongoDatastore; Task(String id, Datastore mongoDatastore) { this.id = id; this.mongoDatastore = mongoDatastore; } @Override public void run() { Sample result = mongoDatastore.find(Sample.class).filter(Filters.eq("_id", id)).first(); if(result == null){ System.out.println("Result is null"); }else { System.out.println(result.toString()); } } } 3. Write main method, create ExecutorService and try to load different documents. ExecutorService executor = Executors.newFixedThreadPool(10); executor.execute(new Task("ABC12345", mongoDatastore)); executor.execute(new Task("ABC123456", mongoDatastore)); I get Two entities have been mapped using the same discriminator value. Try two scenario 1. When ids are valid id from collection 2. Try some invalid ids where collection return null.
MKaramavrov commented 3 months ago

In our project, we faced the same issue with lazy mapping of the entities during runtime. Version, which we currently use is 2.4.11.

evanchooly commented 3 months ago

I'm hesitant to do too much thread-safety work here as this is a very common method to call. A better fix, on your end, is to not rely on lazy mapping (especially as that is going away) and explicitly map your classes/packages up front. This should eliminate your issues. Please try pre-mapping and see if your issue goes away. If it doesn't, we can talk about this PR.

MKaramavrov commented 3 months ago

That was already a discussion here: https://github.com/MorphiaOrg/morphia/discussions/2802 Would be nice to have a fix at least for 2.4.x version, because we've just migrated from version 1.6.x to 2.4.11 and we are currently trying to preregister every entity on startup, but there is conceptual issue, because morphia does not enforce preregistration and during code change someone can miss some entity which will lead to hard to catch and reproduce issue.

MKaramavrov commented 2 months ago

@evanchooly Could we please consider fixing the issue at least for 2.4.x versions? Not sure that we will have too much thread safety, as we are checking for presence of existing model before making it synchronized:

if (existing != null) {
            return existing;
        }