Deadlock closing app after failed tile request

[springmeyer]

To replicate:

• launch the app, hit s until you get to the mapbox-streets style.
• edit ./style/style/mapbox-streets-v7.json and give a phony url for one of the layers:

diff --git a/styles/mapbox-streets-v7.json b/styles/mapbox-streets-v7.json
index 565acd2..d83e0fc 100644
--- a/styles/mapbox-streets-v7.json
+++ b/styles/mapbox-streets-v7.json
@@ -168,8 +168,8 @@
   },
   "sources": {
     "mapbox://mapbox.mapbox-streets-v6": {
-      "url": "mapbox://mapbox.mapbox-streets-v6",
-      "type": "vector"
+      "type": "vector",
+      "url":"http://something.thatdoesnotexist.net"
     },
     "mapbox://mapbox.mapbox-terrain-v2": {
       "url": "mapbox://mapbox.mapbox-terrain-v2",

• launch the app and just the terrain will load
• then try to close the app and it will not close since it will be hung in a deadlock

Seen with mac osx and 21c419b. The Activity Monitor callstack looks like: https://gist.github.com/springmeyer/b39debea42daa9f3e472

[kkaefer]

Looks like the request never gets canceled. This is a regression of 9781785 which was removed again in #1336

[kkaefer]

This regressed as part of #1065; we are no longer sending an explicit termination message to the Map thread, so we never destruct ~MapContext, never destruct ~Source, never cancel the request.

[jfirebaugh]

~MapContext is called when the Thread object goes out of scope. Something else must be going on, I'll investigate.

[jfirebaugh]

Ah, I think I see what you mean about an explicit termination message @kkaefer. I think that should be avoidable with the current design though, where destructors get called through the course of shutting down the thread.

The problem in this scenario is that MapContext's run loop doesn't exit because there are referenced uv_handles belonging to these objects and this async [edit: just the latter; the other objects are handles on a different run loop]. If I cause these handles to be unref'd, the deadlock is resolved. However then I hit an assertion failure in ~HTTPContext(). Continuing to investigate.

[jfirebaugh]

The assertion failure is because, even with unref'd handles, ~Source is not getting called. This is due to Source::load's use of shared_from_this(), the result of which is bound in the lambda. This forms a reference loop: the lambda holds a reference to shared_ptr<Source>, preventing ~Source, preventing the request from being cancelled, preventing the lambda from being released. Fortunately shared_from_this() is unnecessary.

[jfirebaugh]

Further untangling is running into the known issue that cache requests can outlive their completion callback and also the complexities of the DefaultFileSource::cancel flow. I think I'll need to fix both of these issues in order to fully resolve this, but I know that @kkaefer was also working on cancellation. @kkaefer, how is that looking?

[springmeyer]

@jfirebaugh curious, is #1629 a step towards resolving this or unrelated?

[jfirebaugh]

I thought it would be a step towards resolving this but in the end it's unrelated.

[tmpsantos]

#1588 and #1632 are adding an error notification to renderStill(). We could (and probably should) use the some code to send error notifications on the continuous mode.

[mikemorris]

I think I'm hitting a variation of this at #1588 (comment), this is sorta blocking merging a fix to https://github.com/mapbox/node-mapbox-gl-native/issues/122 now.

[tmpsantos]

@jfirebaugh do you mind if I take this one? I'm on #1657 which is almost the same thing (if not the same).

[jfirebaugh]

Go for it. I think #1657 is the same. This async handle keeps the map thread event loop alive.

[tmpsantos]

Fixed by #1657