Map object destruction is hanging #1657
Conversation
|
/cc @mikemorris |
tmpsantos
added
bug
⚠️ DO NOT MERGE
|
Progress update: the resource loading is starting at the moment we set the style instead of when we call |
tmpsantos
force-pushed
the
1657-map_object_hanging_on_dtor
branch
from
4b9b1d9 to
1cc33ee
Compare
tmpsantos
added
ready
and removed
in progress
⚠️ DO NOT MERGE
tmpsantos
force-pushed
the
1657-map_object_hanging_on_dtor
branch
from
1cc33ee to
48bd2a8
Compare
|
@jfirebaugh @kkaefer please take a look at this on too |
|
@tmpsantos Still seeing hangs as a race condition when testing node-mbgl against this. When three requests emit errors to the log and then the error callback is returned and the destructor is called, the process hangs. When all four requests emit errors before the callback returns, the process exits cleanly. I'm testing this by enabling the |
| @@ -47,6 +47,14 @@ MapContext::MapContext(uv_loop_t* loop, View& view_, FileSource& fileSource, Map | |||
| } | |||
|
|
|||
| MapContext::~MapContext() { | |||
| if (resourceLoader) { | |||
| // This is likely to be called only on unit tests when | |||
| // the MapContext is not bound to a Map object. | |||
There was a problem hiding this comment.
Can we avoid code that is specific to unit tests? This sort of defeats the purpose of having unit tests when we special case them in our regular code.
There was a problem hiding this comment.
Can we avoid code that is specific to unit tests? This sort of defeats the purpose of having unit tests when we special case them in our regular code.
I agree. Gonna change this.
|
In what cases do we not have a |
tmpsantos
force-pushed
the
1657-map_object_hanging_on_dtor
branch
from
48bd2a8 to
df07fce
Compare
We start loading the resources at the moment we set the style, but we set the callback for error notification at renderStill(), which happen afterwards. If some error occurs in this short window, it was never being notified. Now we save the last error and we check after we set the callback.
The util::Thread will call the stop() method of the MapContext's RunLoop which will wait for the pending tasks tied to it to complete. If we have a request that is timeout'ing, this could mean a really long wait. Instead, we now send a cleanup message that will reset all the attributes first (canceling the pending requests) so the the MapContext gets destructed quickly.
Test whenever destroying a Map object with requests pending works and is done quickly. The test injects artificial delays in selected requests and tries to destroy the Map object afterwards. Currently glyph requests are skipped because there is a bug being worked on in a different issue.
tmpsantos
force-pushed
the
1657-map_object_hanging_on_dtor
branch
from
df07fce to
1408370
Compare
|
👍 Did this fix #1477? |
Yes! 😄 |
Under some scenarios, destroying the Map object is hanging. This is specially easy to reproduce if we trigger rendering and when a request fails with a few other pending, we delete the Map object.
This is probably happening because there are pending requests holding the Map Thread's main loop alive and deadlocking somewhere. In the past we had a method for canceling all the requests that belong to an Environment, but now we need to make sure that every object doing requests is canceling pending requests at the dtor.