search

erlanglab / erlangpl

Tool for developers working with systems running on the Erlang VM (BEAM). It helps with performance analysis.
http://www.erlang.pl/
Apache License 2.0
549 stars 40 forks source link

Make supervision tree fetching more error proof #57

Open gomoripeti opened 7 years ago

gomoripeti commented 7 years ago

If a child is wrongly specified as supervisor, but it does not implement this behaviour, the supervisor:which_children/0 call can return unpredictable values. (Especially if the worker is a gen_server which can return any sort of funny value in its handle_call).

This issue has been seen in more than one open-source library.

Hopefully closes #37

baransu commented 7 years ago

So if some branch of sup-tree is not OTP correct for example it would skip whole branch?

gomoripeti commented 7 years ago

I think in most cases a worker child is incorrectly declared as a supervisor child so there are no grandchildren and it is safe to handle this process as a leaf. If this process is a custom parent which has children but does not implement the which_children api then, yes, the subtree under it would be ignored. But in this case epl would have a hard time to figure out grandchildren anyway. (I only know of rabbitmq which has its own supervisor implementation, but it also supports the which_children call)

baransu commented 7 years ago

We want to enforce correct OTP structure so rather throw error than ignore something incorrect.

/ cc @michalslaski

arkgil commented 7 years ago

Or instead of throwing an error we could show an information in the UI that this branch of the tree cannot be resolved.

gomoripeti commented 7 years ago

It makes total sense not to hide the discrepancy. The easiest way would be to just crash with a very descriptive error message. But it would be more user-friendly to either print an error message (only once and not every second when the sup-tree is updated) or indicate this in the UI somehow. If it is not just a crash I would like to leave the implementation to you guys.