Tronald / CoordinateSharp

A library designed to ease geographic coordinate format conversions, and determine sun/moon information in C#
Other
364 stars 59 forks source link

Un-representable during multithreading #86

Closed emildayan closed 5 years ago

emildayan commented 5 years ago

First of all - great plugin! Thanks a ton for making it.

Getting an unfortunate error though when running my app multi-threaded.

Any idea how to resolve? Thanks! Emil

var NE = new CoordinateSharp.Coordinate(center.Latitude, center.Longitude);
NE.Move(0.5* 1000, -45, Shape.Sphere); <-- Error line
System.ArgumentOutOfRangeException: 'The added or subtracted value results in an un-representable DateTime.
Parameter name: value'
at System.DateTime.AddTicks(Int64 value)
   at CoordinateSharp.SolarEclipseDetails..ctor(List`1 values)
   at CoordinateSharp.SunCalc.CalculateSolarEclipse(DateTime date, Double lat, Double longi, Celestial c)
   at CoordinateSharp.SunCalc.CalculateSunTime(Double lat, Double longi, DateTime date, Celestial c, Double offset)
   at CoordinateSharp.Celestial.CalculateCelestialTime(Double lat, Double longi, DateTime date)
   at CoordinateSharp.Coordinate.NotifyPropertyChanged(String propName)
   at CoordinateSharp.CoordinatePart.NotifyProperties(PropertyTypes p)
   at CoordinateSharp.CoordinatePart.set_DecimalDegree(Double value)
   at CoordinateSharp.Coordinate.Move(Double distance, Double bearing, Shape shape)
   at GoogleMapsScraper.ViewportCoordinates..ctor(Coordinate center, Double kmRadius) in D:\Downloads\code\GooglePlace.cs:line 342
   at GoogleMapsScraper.Coordinate.GetViewpointCoordinates(Double kmRadius) in D:\Downloads\code\GooglePlace.cs:line 400
   at GoogleMapsScraper.Coordinate.ViewportUrl() in D:\Downloads\code\GooglePlace.cs:line 391
   at GoogleMapsScraper.Program.Scrape(WebClient client) in D:\Downloads\code\Program.cs:line 55
   at GoogleMapsScraper.Program.<>c__DisplayClass5_0.<Main>b__0() in D:\Downloads\code\Program.cs:line 37
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx)
   at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx)
   at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state)
   at System.Threading.ThreadHelper.ThreadStart()
Tronald commented 5 years ago

Thank you for bringing this to my attention, and your kind words.

I just stress tested your code and was unable to replicate, so I have a few questions.

  1. What version of the library are you using?
  2. Can you show me how you're starting your threads.
  3. Do you need Celestial Calculations? If not, turning them off will provide a workaround for your issue (I can explain how to do so if this is the case).

Thanks!

emildayan commented 5 years ago

Thanks a lot for the quick reply!

1) 1.1.4.4 (from NuGet) 2) Using System.Threading thread = new Thread(() => Scrape(client)); thread.Start();

3) I'll be honest and say I don't know what the Celestial Calculations do, but here's the full code.

var NE = new CoordinateSharp.Coordinate(center.Latitude, center.Longitude);
                NE.Move(kmRadius * 1000, -45, Shape.Sphere);
                var SW = new CoordinateSharp.Coordinate(center.Latitude, center.Longitude);
                SW.Move(kmRadius * 1000, 135, Shape.Sphere);
                NorthEast = new Coordinate
                {
                    Latitude = NE.Latitude.DecimalDegree,
                    Longitude = NE.Longitude.DecimalDegree,
                };
                SouthWest = new Coordinate
                {
                    Latitude = SW.Latitude.DecimalDegree,
                    Longitude = SW.Longitude.DecimalDegree,
                };

So I'm only using it to calculate correct NE/SW.

Tried to do this manually but got incorrect results depending where on the map the base(center) coordinate were, and your code handled that just fine - I'm not sure if removing Celestial would change that?

Tronald commented 5 years ago

Thank you for the update. I would like to investigate the bug further when I have time this afternoon, so I may reach back out. With that said, because you don't need any celestial, UTM or Cartesian calculations you can turn them off by passing an EagerLoad object in your constructor to turn off EagerLoading of those items (thats a mouthful lol).

Change:

var NE = new CoordinateSharp.Coordinate(center.Latitude, center.Longitude);

To

var NE = new CoordinateSharp.Coordinate(center.Latitude, center.Longitude, new EagerLoad(false));

Let me know if that fixes your issue. This is only a workaround and will not work if you decide you need to EagerLoad those items in the future, so hopefully we can figure out what's going on.

Thanks again.

-Justin

emildayan commented 5 years ago

Thanks Justin! I'll give that a shot when I'm in the office tomorrow and will let you know!

Again - many thanks for your work on this. A true savior for our project! Emil

Tronald commented 5 years ago

Thank you @emildayan

I cannot for the life of me get this bug to replicate. The Eclipse classes use List so those calculations wouldn't be thread safe, but as long as you aren't accessing the same coordinate object at the same time this shouldn't matter. I'm perplexed.

  1. What .NET version are you using?
  2. What kind of application are your running the library in?
  3. Is the error occurring when specific coordinates are passed, or does it throw for every coordinate?

Do you think you could get the error to throw with a minimal code example in a console application?

Any help is greatly appreciated.

Tronald commented 5 years ago

@emildayan disregard my last comment. I finally able to replicate by editing the same Coordinate object from 2 different threads at the exact same time.

I didn't design the library with thread safety in mind, so I am not sure how deep this rabbit hole goes. For your purposes however, the work around should remain sufficient.

Thanks again.

Tronald commented 5 years ago

I removed the static variables from LunarEclipseCalc and SolarEclipseCalc classes and instead pass them through functions in 1.1.4.5. I plan to release in the next few days.

I believe the threading issue stems from dual access of those variables. Removing them all together should hopefully stop that. Race testing is hard to do reliably however, but this specific issue seems to be no longer replicating. Thanks for bringing this up.

Tronald commented 5 years ago

1.1.4.6 is released (available via Nuget) and should fix your issue. It should also fix any errors occurring with using a 'Move()' command with EagerLoading off.