Mapsui: Problem with crash on android LayerCollection.CopyTo

[johnhs91]

Hello

We've recently updated app to run Mapsui forms. Had my own custom renderes before.
After this update we got some crash. (I have not seen it myself, but I see it in app center)

Error:
LayerCollection.CopyTo (Mapsui.Layers.ILayer[] array, System.Int32 arrayIndex)
System.ArgumentException: The number of elements in the collection is greater than the available space from index to the end of the destination array.

Stack trace:

ConcurrentQueue`1[T].CopyTo (T[] array, System.Int32 index)
LayerCollection.CopyTo (Mapsui.Layers.ILayer[] array, System.Int32 arrayIndex)
System.Collections.Generic.List`1[T]..ctor (System.Collections.Generic.IEnumerable`1[T] collection) [0x0003b] in <19853c43ab794d18ab1a33ecb65b3c4d>:0
Enumerable.ToList[TSource] (System.Collections.Generic.IEnumerable`1[T] source)
Map.RefreshData (Mapsui.Geometries.BoundingBox extent, System.Double resolution, System.Boolean majorChange)
MapControl.RefreshData ()
MapControl.Refresh ()
MapControl.MapPropertyChanged (System.Object sender, System.ComponentModel.PropertyChangedEventArgs e)
Map.OnPropertyChanged (System.Object sender, System.String propertyName)
Map.OnPropertyChanged (System.String name)
Map.LayersLayerRemoved (Mapsui.Layers.ILayer layer)
(wrapper delegate-invoke) <Module>.invoke_void_ILayer(Mapsui.Layers.ILayer)
LayerCollection.OnLayerRemoved (Mapsui.Layers.ILayer layer)
LayerCollection.Remove (Mapsui.Layers.ILayer layer)
MapView.RemoveLayers ()
MapView.HandlerLayerChanged (Mapsui.Layers.ILayer layer)
(wrapper delegate-invoke) <Module>.invoke_void_ILayer(Mapsui.Layers.ILayer)
LayerCollection.OnLayerAdded (Mapsui.Layers.ILayer layer)
LayerCollection.Add (Mapsui.Layers.ILayer layer)
MapSuiView2+<>c__DisplayClass103_1.<AddMarker>b__1 ()
Thread+RunnableImplementor.Run ()
IRunnableInvoker.n_Run (System.IntPtr jnienv, System.IntPtr native__this)

Have no clue how to generate this crash. Why it appears

Have anyone seen this before? And maybe know a fix for it

[pauldendulk]

This is because there is a workaround in the mapview for the fact that mapsui has no solution to keep specific layers on top.

[johnhs91]

Don't think I undestand.
It's a bug that mapsui can't keep layers on top. But workaround in mapview how ?

[pauldendulk]

There is no functionality in the Mapsui MapControl to keep a certain set of layers on top, like the gps location and the pins. Because of this limitation the MapView which is built on top the MapControl removes all layers and re-adds them (on top) each time a layers is added. This is the workaround to keep these layers on top. This causes complications.

[johnhs91]

Now I understand. Make sense.

[charlenni]

Do you have any other layers that you add? What do your app do?

For me it looks like you are adding an Layer to the layers. Is this correct? There is also a AddMarker? Correct?

[charlenni]

Now I understand. Make sense.

Shouldn't make any problem.

[johnhs91]

Problem is I got a layer per marker I show on the map.
So every time I add a marker. I add another layer to the map.
Reason they are in seperate layer, is because I need to be able to change color invidual on marker.

And if I create layer with all markers, I can't change one single marker color.

So I only got tile layer + layers for polygon, line and point(Marker)

[charlenni]

Diod you look at the PinSample.cs? There you could see, how to make a individual color for each pin. Or use styles for feature if you want to make it by yourself.

[johnhs91]

Maybe it's possible today, wasn't when I started using it. Don't really have time to change now.

I maximum have 2 layers with one feature on each as is today.

I have a clue about why the crash appear. But we have to agree it shouldn't in this case.
I should always be able to add layers on the map on the fly. Can't see why this should make it crash.

[charlenni]

I have a clue about why the crash appear. But we have to agree it shouldn't in this case.
I should always be able to add layers on the map on the fly. Can't see why this should make it crash.

Yes. So it would be wonderfull, if you could create a small sample to reproduce this problem.

[johnhs91]

Wish I could.
200 users use the app daily. And it only crash 1 time a day.

I've tried to reproduce myself without luck.
I will try to reproduce over next weeks. And see if I can find a workaround.

Thank you for fast reply.

[pauldendulk]

We know it's a problem. We have to think of a real solution for the problem.

[pauldendulk]

@charlenni If we implemented this interface could the MapView handle the adding of user layers without removing and re-adding all it's own layers?

https://github.com/Mapsui/Mapsui/issues/598#issuecomment-470975342

Users will have to understand something about grouping. If they add to group 0 you still have the same problem. I would think that's acceptable.

[pauldendulk]

Users will have to understand something about grouping. If they add to group 0 you still have the same problem. I would think that's acceptable.

Perhaps we could allow groups smaller than 0. So the gps layer could be in group -1. If the users do not specify the group (default 0) it will still be underneath the layers in group -1.

[charlenni]

No. MapView is a low knowledge interface to Mapsui. If you have understand Mapsui, than you don't need MapView. If you not have understand Mapsui, than you don't understand, why grouping of layers is needed.

So I see no problem removing and adding the layers. This workaround works for me. But we could change the LayersCollection and integrate this functionallity. But this isn't a bug fix.

[pauldendulk]

This workaround works for me.

😮

What kind of a response is that 🙄. The application crashed but it works on my machine.

My last response suggest the user does not have to have knowledge. The MapView adds the layers to group -1. The user will just add a layer like he always did. How about that?

[charlenni]

Sorry, but the thread owner didn't mentioned anything about the version of Mapsui he uses and no comment, if this is only a special device problem. Is the layer added in the UI thread or in the background? So where to start? Should change the complete process of creating layers, because there is a sometimes problem?

I would say, Mapsui has a problem adding and removing layers. MapView uses this functions. While removing the layer, a refresh is triggered and it seems, that this refresh isn't handled correct.

So where to start? At System.Collections.Generic.List or ConcurrentQueue?

[pauldendulk]

I investigated the relevant code before. It is problematic. This is the place where the MapView got into a loop of adding/removing. And by changing something in the threads this could then be solved.

The problem is that the MapView removes and adds layers in the the LayerAdded and LayerRemoved event of the LayerCollections. See the problem? While the LayerCollection is adding layers a LayerAdded event is called and in that event all layers are added and removed. I am surprised that this actually succeeds something.

c# _mapControl.Map.Layers.LayerAdded += HandlerLayerChanged; _mapControl.Map.Layers.LayerMoved += HandlerLayerChanged; _mapControl.Map.Layers.LayerRemoved += HandlerLayerChanged;

[charlenni]

If a layer is added, than I remove all layers and add them again. This happens only, if they are not from MapView/MapControl. See here.

[pauldendulk]

I would say, Mapsui has a problem adding and removing layers. MapView uses this functions. While removing the layer, a refresh is triggered and it seems, that this refresh isn't handled correct.

So where to start? At System.Collections.Generic.List or ConcurrentQueue?

Mapsui uses indexes in the interface of the LayerCollection (In Insert, Move and CopyTo). It inherited this from SharpMap. Calling such an interface from multiple threads I guaranteed to cause exceptions like this at some point. A proper solution would be to create something with relative positions like in the interface suggested above.

[pauldendulk]

@johnhs91 Since the LayerCollection uses indexes in its methods it is hard to avoid problems when it is called on different threads at the same time. I just pushed a commit where I added some logic to defend against this. However, the measures taken in the CopyTo method might have other unwanted consequences. An array of a certain length is passed in, in order to copy to it. The array usually has the length of the whole layerCollection. It could be that before the copy action in the CopyTo method a layer is removed from the list (not likely, but that is happening in your case). What I did to deflect that is to check on the size of the array and layerCollection and copy only the part that fits. This avoids an exception but what I get as a result is an array where some elements can be null. This is my test: https://github.com/Mapsui/Mapsui/blob/master/Tests/Mapsui.Tests/Layers/LayerCollectionTests.cs#L53

This is a problem too. A LayerCollection.ToList might contain a null value. Users will need to start checking for null and it would become possible that some layers disappear from the map.

Now looking at the stack trace I wondered what was going on. Apparently CopyTo is called from a List constructor. And this in turn is called from the ToList in Map.RefreshData. So a simple ToList causes CopyTo to be called. What I assumed is that this is a performance optimization in the List constructor which explicitly checks on the ICollection interface. Looking at the C# source code this indeed the cast:
https://github.com/microsoft/referencesource/blob/master/mscorlib/system/collections/generic/list.cs#L79

What I did now is to derive the LayerCollection from IEnumerable instead of ICollection to avoid this case, and avoid the call to CopyTo. This might affect performance but will only be noticeable with a large number of layers.

[charlenni]

Why not use a concurrent version of collection?

[pauldendulk]

@johnhs91 I just released 2.0.0-beta.34. I hope you can test it.

The fix could have impact on adding and removing layer performance. You mention you create a layer for each feature for styling. This is not necessary. Individual features can be styles in various ways.

[pauldendulk]

@charlenni The LayerCollection was already using the ConcurrentQueue(). This collection still has a CopyTo method and when there are more items in the ConcurrentQueue than in the array it crashes.

In this unit test I added a scenario:
https://github.com/Mapsui/Mapsui/blob/research/concurrentqueue-copyto/Tests/Mapsui.Tests/Layers/LayerCollectionTests.cs#L176

So a ConcurrentCollection can still be crashed by the caller if the wrong array length is used as argument.

btw, it crashes when the source (the collection itself) is larger than the array, not the other way around.

[johnhs91]

I will update for next app version next week. And make a few tests.
I don't have much time to test these weeks.

[johnhs91]

You asked what version I use. it's 2 beta 30.

[pauldendulk]

I consider this fixed.