Object Graph circular dependency check is not thread safe

[dalewking]

Was experiencing some occasional, random array index out of bounds exceptions from circular dependency check in graph validation and looking at the code it occurs to me that the code for circular dependency checking is not thread safe. It sets a visiting flag on the binding to mark that it is currently visiting it. But if another thread happens to be validating the same graph this can lead to problems. If a binding is visited on another thread the current thread will see the binding as visited. When it tries to compose a nice message of the cycle it starts at the index of the binding in its path, but the current path has not actually visited it so does not have that binding in its path thus the Array index out of bounds exception.

In our case the multiple threads was due to the fact that we use Dagger to also inject some services and it was always the injection for a service that failed. If they happened to be started at the same time as an injection for the UI we get the exception.

[dalewking]

For some reason in our code, we were validating the graph on each injection (in debug mode). I moved it to only validate after the graph is created which means this is not a high priority thing to fix, but this will serve as notification in case someone else runs into this.

[cgruber]

The runtime graph validation is only a hedge against adding a different set
of modules at runtime vs. at compile time. If you are certain you are
passing in the same modules that you compiled with, then the validation you
are running has already occurred. It's a safety vs. performance question -
but there it is.

On Sat Jan 31 2015 at 7:16:52 PM Dale King [email protected] wrote:

For some reason in our code, we were validating the graph on each
injection (in debug mode). I moved it to only validate after the graph is
created which means this is not a high priority thing to fix, but this will
serve as notification in case someone else runs into this.

—
Reply to this email directly or view it on GitHub
#459 (comment).

[dalewking]

As I said we were only doing the validation in debug mode. If there were issues we wanted to discover them during development. But as I said doing the validation for every injection was a bad idea.

[cgruber]

Acknowledged.

On Mon Feb 02 2015 at 4:30:03 PM Dale King [email protected] wrote:

As I said we were only doing the validation in debug mode. If there were
issues we wanted to discover them during development. But as I said doing
the validation for every injection was a bad idea.

—
Reply to this email directly or view it on GitHub
#459 (comment).