In an unlikely situation a user of ext_asio may tamper with Continuation
before passing it to the ext_asio extension. Let's fail with an
exception if this happens. Previously, a user bug would stay unnoticed,
but would not harm the ext_asio code.
This check will be needed once we implement optimistic execution. With
optimistic execution, we iterate continuation and defer construction of
ContinuationWaitHandle until the first blocking event occurs. During
this phase, a standard dependency loop detection is skipped and the code
would try to iterate a continuation that is already being iterated.
We only used it to get the values of certain class constants,
and to define the ObjectStaticCallbacks for every class.
We can put the class constants directly into the class_map
(we should have done that before for perf reasons), and then
the only remaining use of ObjectStaticCallbacks is to proxy
the Class* for each builtin class. So just use the Class*
directly.
Once this is in, Im just a small step away from eliminating
make -C hphp/system - so Im leaving a lot of dead code here.
Its going to be easier to delete it en masse, rather than
try to pick and chose now.
enterContext() throws an exception when cross-context cycle is found.
The problem is that it modifies state before the exception is thrown,
assuming that the call will succeed.
When an exception is thrown, a dependency is left in invalid state, with
parent being in more specific context. This breaks exitContext()
algorithm and results in either internal invariant violations as seen
in #2091939, or memory corruptions and crashes as seen in #2125762.
Let's fix it by modifying state after returning back from recursive call
instead of before doing such call. This was previously unsafe in case we
tried to import dependency loop. Once D720506 is committed, dependency
loops will not exist anymore.
Currently, we detect dependency loops by waiting until there is nothing
else to execute. If the wait handle we are waiting for did not finish,
it means it is in a cycle. We find the cycle by simply following the
dependency chain. Once the cycle is found, one edge is eliminated and an
exception is injected.
There are multiple problems with this approach:
1. Unability to exit contet safely
We are unable to exit context safely. When a context is exited, all wait
handles in that context must be kicked out. But we maintain only
references to the SCHEDULED wait handles + BLOCKED wait handles that
recursively depend on them.
If we do not kick out all unfinished wait handles, we end up in
corrupted state.
2. Unability to break edge that caused the cycle
Once the cycle is detected, we don't know which edge caused the cycle to
be formed. We can only use heuristics to eliminate the edge that likely
formed the cycle, we cannot be sure. This may make it very hard to fix
the PHP code that caused the cycle.
Solution:
This diff implements online cycle detection with a naive approach of
visiting the dependency chain from child at a time new edge between
parent and child is being added. If a parent is visited, a cycle is
found. Otherwise we eventually reach non-BLOCKED wait handle as it is
guaranteed the rest of the graph is cycle-free.
Currently, wait handles store pointer to the context they are in. This
pointer is not protected with reference counting, as it is expected that
whenever a context is exited, references to it are cleaned thru
exitContext() mechanism.
If a bug is present that violates this assumption, it is impossible to
guard against invalid pointer access and a hard to debug memory
corruption occurs.
Since the structure of contexts is a simple stack, let's reference them
by index instead of by pointer.
As a bonus, one pointer worth of memory is saved for every non-trivial wait handle.
The actual bugs will be fixed by the next 2 diffs that do:
1. implement online cycle detection
2. do enterContext() atomically and properly handle failure
Avoid dynamic_cast<> on fast paths, use o_instanceof(StaticString)
instead. Saves about ~0.2-0.3% of CPU time.
Once HPHPc is gone, we will convert these calls to HHVM-specific API.
Thanks @bmaurer for discovery and @mwilliams for suggestion how to fix
it.
This change is mostly for FB internal organizational reasons.
Building is not effected beyond the fact that the target now
lands in hphp/hhvm/hhvm rather than src/hhvm/hhvm.
jan