John Doe, Why not use the session? That is what it is there for, and it makes things much easier. You can use it as a one off, for firing a single query. I have issues with that, but much less than the pattern above.

John Doe 2, Nope, the mere fact that this is possible or of concern to you indicates a huge architectural problem. The lock is an indication that someone thought that this is possible, which indicate a whole level of issues that needs to be addressed first. It is like saying, "watch out about the loose, hungry, tiger in the nursery" and not doing anything to fix it.

Session management is supposed to flow the unit of work pattern, with very clear boundaries on its usage. There should be no case in which this is even possible, certainly not of conern.

When providing code to others, you need to be clear about your expectations on usage.

John Doe 2, Pretty much all databases that I'm aware of have strict rules about the concurrency of session / connection / etc. A repository instance can be safely assumed to be non thread safe across a whole range of technologies.

In fact, given thread safeties issues in general, assuming that a repository of any kind is thread safe is a really bad default, as is any assumption that code needs to be multi threaded safe by default. In fact, this is the other way around, code is single threaded by default and you need to have explicit documentation that it is safe to use from multiple threads. A big workload that is used on multiple threads is a sign of bad architecture. If you need a unit of work that compose multiple threads, you need to have the architecture to support it. At a glance, I would do:

• First stage, load relevant details.
• Run multiple threads processing it. <-- No database access is allowed here in the same unit of work. You may open a smaller, dedicated one, but not use a shared one.
• Last stage, commit the changes.

As for "no control over whether they'll share a repository". You also have no control over whatever they do error handling, and yet I assume you still use exceptions? If a user insist that they never call the new operator, will you design a system around that?

Being thread unsafe by default is not something weird or extra ordinary. This is the natural order of things, and users who make baseless assumptions gets exactly what they are deemed worth.

We made an architectural decision in our particular product that we would make our repositories thread safe

That is a mistake. I know that this may sound harsh, but is is a big mistake. Literally no other product on the market has session / connections that are safe for multi threading. Your entities are not likely to be thread safe and the whole thing is going to require you to take upon yourself a lot of really complex stuff for what is likely to be marginal benefit. A proper architecture would give you the benefit of multi core scaling without the requirement to handle thread safety issues in this manner. Again, note that there is a big difference between thread safe and scaling up with parallelism via concurrent instances.

That is leaving aside the fact that you are trying to achieve higher performance but to ensure that the system operates you have to slap locks on multiple places that is going to be a perf killer.

In our system then robustness is important.

That kind of system isn't robust. See: https://ayende.com/blog/180385/pr-review-encapsulation-stops-at-the-assembly-boundary

This is trying to hide (a very serious) issue by limping along. For that matter, this kind of thinking exposes you to Hyrum’s Law: “With a sufficient number of users of an interface, it doesn’t matter what you promised in the interface contracts, all observable behaviors of your class or function or whatnot will be depended upon by somebody.”

That leads you from an outright error to having to try to support it, because you have sufficent number of users who assume that this is safe, and you can't make it safe (at least not with good performance) in the general case.

there is rare code in our product that uses it from another thread, then it doesn't crash.

You want it to crash. Crashing is a godsend in these cases. What you are describing is a race condition, and the last thing you want it to do is to continue running. Because that way you have madness. You have concurrently executing code that is doing strange stuff and violates a lot of assumptions in your domain in very fundemental manners.

Each thread may need to query the DB occasionally and so it will need to have access to the repository.

Yes, absolutely agree with you up to the last two works. It isn't _the repository_. It is _the repositories_. A repository is stateful, and each thread should have one for the scope of the operation.

Your recommendation that each thread uses their own repository may not be practical.

You want to debug this kind of code? Because I have done many mutli threaded debugging, and it is harsh. With RavenDB 4.0 we have made multiple passes to reduce this kind of sharing as an explicit goal to make debugging and operations easier.

The method caller passed in a reference to a single repository.

Fix the method, then. You'll regret it otherwise.

Let us take your example and show the issue: "For example, You have a method that accepts a file name and a single repository reference, uploads the file to 10 servers as quickly as possible and then updates a flag in Raven to indicate that the process is complete for the particular server.".

We'll assume that you have code similar to this:

public void UploadToRemoteServers(string file, IRepository repos)
{
    // error handling / etc elided
    // explicitly lockign the repository to show the issue, but same if the repo handle it internally

    Server[] servers;
    FileRecord file;
    lock(repos)
    {
        servers = repos.FindServersFor(file);
        file = repos.CreateFileRecord(file);
    }

    Parallel.ForEach(servers, server =>{

        lock(file)
        {
            file.AddServer(server.Uri);
        }
        UploadToServer(file, server); // can take a while
        lock(repos)
        {
            repos.UpdateFileRecord(file);
        }
    });
}

There are a few things wrong here. At first glance, this code may look proper, but it has multiple race conditions. You can get a record about the file upload before it is finished uploading or even if it failed to upload. The problem is apparent if you are using explicit locks, but what about this code? Now "properly" separated.

public void UploadToRemoteServers(string file, IRepository repos)
{
    // error handling / etc elided
    // explicitly lockign the repository to show the issue, but same if the repo handle it internally

    Server[] servers = repos.FindServersFor(file);
    var fileId  = repos.CreateFileRecord(file);

    Parallel.ForEach(servers, server =>{
        UploadToServer(file, server); // can take a while
        repos.UpdateFileRecord(fileId, server);
    });
}

// in the repository

public void UpdateFileRecord(string recordId, Uri serverUri)
{
    var fileRecord = Get<FileRecord>(recordId);
    fileRecord.AddServer(fileRecords);
    StoreAndSaveChanges(fileRecord);
}

public T Get<T>(string id)
{
    lock(_session)
        return _session.Load<T>(id);
}

public void StoreAndSaveChanges(object entity)
{
    lock(_session)
    {
        _session.Store(entity);
        _session.SaveChanges();
    }
} 

At a glance, this looks fine, proper locking, etc. But you are potentially mutating the same file record concurrently, that can lead to error, hangs, etc. If the AddServer method modify a dictionary, you can get into an infinite loop quite easily, for example, if there is a race.

This is code that is easy to write, easy to read and really easy to miss the error if you don't consider it. It can also run, in production, for years on end, without any trouble. And then one day the entire world will come crashing down on your program because of something like this. The most recent example is here: https://ayende.com/blog/180769/production-postmortem-the-random-high-cpu

That particular codepath hasn't changed in literally years, it is deployed to tens of thousands or higher of nodes and gave us no trouble for years. To trigger it, an operation that should take 1 second should take more than 30 _minutes_. And somehow, it was triggered. For that matter, it was triggered several times in a particular environment.

Being thread-safe is a perfectly legitimate architectural decision

Yes, it is. In the right place. A repository is not that place. You are taking an extremely high cost upon yourself with this kind of attempt, for very few benefits and opening a whole huge can of worms.