NtsGeometryServices is not thread safe, but is used as a singleton

[GoogleCodeExporter]

NtsGeometryServices.Instance is a singleton instance that can be used from 
multiple threads at the same time.
However, it isn't thread safe and can cause errors when used from multiple 
threads at the same time.

In particular, NtsGeometryServices._factories is used in a non thread safe way 
in CreateGeometryFactory() method.

In fixed that by changed _factories to be ConcurrentDictionary, rather than a 
Dictionary, which solved the problem.

Original issue reported on code.google.com by aye...@ayende.com on 20 Sep 2012 at 11:46

[GoogleCodeExporter]

ConcurrentDictionary resulted in having to move to .NET 4.0 - which was a major 
change.
You can also change that by doing double checked locking on standard Dictionary.

Original comment by aye...@ayende.com on 20 Sep 2012 at 4:11

[GoogleCodeExporter]

thanks for reporting this. AFAIK, and noticed that you're "that" ayende, do you 
think that there can be some performances issues using ConcurrentDictionary or 
double locking?

Original comment by diegogu...@gmail.com on 20 Sep 2012 at 5:31

[GoogleCodeExporter]

It doesn't matter whatever CD or double locking are slow.
Without it, you _cannot_ use this with multi threading, and that is pretty much 
required.
The cost isn't that high, anyway.

Original comment by aye...@ayende.com on 20 Sep 2012 at 5:41

[GoogleCodeExporter]

fixed as of r902. please try ThreadingTest in GeometryServiceProviderTest.
Please note: you may run into more threading related issues. NTS and JTS is by 
no means thread-safe.

Original comment by felix.ob...@netcologne.de on 21 Sep 2012 at 8:19

[GoogleCodeExporter]

This isn't a good fix, you are _always_ locking.
You are probably better of to be:

if (!_factories.TryGetValue(gfkey, out factory))
{
    lock (_factoriesLock)
    {
        if (!_factories.TryGetValue(gfkey, out factory))
        {
            factory = new GeometryFactory(precisionModel, srid, coordinateSequenceFactory);
            _factories.Add(gfkey, factory);
        }
    }
}

Speaking of which, is it even valid to use an IGeometryFactory from multiple 
threads?
I assume that since NTS isn't supposed to be multi threaded, the answer is no.

That being the case, you _cannot_ rely on singletons and things like 
Type.Instance throughout the codebase.

Original comment by aye...@ayende.com on 21 Sep 2012 at 11:12

[GoogleCodeExporter]

Thanks for the suggestion (see r903).

Original comment by felix.ob...@netcologne.de on 21 Sep 2012 at 12:36

[GoogleCodeExporter]

That still leaves the question, can you use the factory from multiple
threads at the same time?

Original comment by aye...@ayende.com on 21 Sep 2012 at 12:39

[GoogleCodeExporter]

Yes, please see/try/enhance NetTopologySuite.Tests.NUnit/IO.WKTReaderTest.cs

The only case -that I know of- that doesn't work in a multithreaded context is 
demonstrated in the 
NetTopologySuite.Tests.NUnit/Performance/Geometries/Prepared/PreparedGeometryThr
eadSafe.cs
test. If you comment the last line in the Setup() routine, the code will not 
work.

Original comment by felix.ob...@netcologne.de on 21 Sep 2012 at 1:25

[GoogleCodeExporter]

Any idea why Intersects will mutate state in this fashion?
Where is the code failing?

Original comment by aye...@ayende.com on 21 Sep 2012 at 2:07

[GoogleCodeExporter]

Note that I run this with a 100 threads a few times, I couldn't get it to
fail.

On Fri, Sep 21, 2012 at 5:07 PM, Oren Eini (Ayende Rahien) <
ayende@ayende.com> wrote:

Original comment by aye...@ayende.com on 21 Sep 2012 at 2:10

[GoogleCodeExporter]

The test does not signal that it is failing, it just aborts.
There should be a lot of output showing progress, but on my machine there is 
none.

The code fails somewhere around instantiating the 
NetTopologySuite.Geometries.Prepared.PreparedPolygon.IntersectionFinder.
This is done in lazy fashion, since it is expensive and not needed for all 
predicates.
The same applies to the EnvelopeInternal computation.

Original comment by felix.ob...@netcologne.de on 21 Sep 2012 at 2:44

[GoogleCodeExporter]

OOPS, it seems like adding WaitHandles show that the code actually does work.

Original comment by felix.ob...@netcologne.de on 21 Sep 2012 at 3:09

[GoogleCodeExporter]

I don't follow, is there or isn't there an issue?

Original comment by aye...@ayende.com on 21 Sep 2012 at 7:44

[GoogleCodeExporter]

There is no issue, I just misinterpreted not getting all results

Original comment by felix.ob...@netcologne.de on 22 Sep 2012 at 9:09

• Changed state: Fixed

[GoogleCodeExporter]

With all due respect, the code suggested by ayende is wrong. That is not double 
checked lock, the TryGetValue for dict is not thread-safe. (I can repro 
IndexOutOfRangeException which is caused by threadng issues while using r903 
method) The first fix (r902) was the right thing to do.

Original comment by predat...@gmail.com on 1 Nov 2012 at 9:06

[GoogleCodeExporter]

@predator4: Thanks, can you please perform your failing test with latest 
check-in?

Original comment by felix.ob...@netcologne.de on 8 Nov 2012 at 10:46