lib: run most of bt_self_component_port_input_message_iterator_try_finalize when...

lib: run most of bt_self_component_port_input_message_iterator_try_finalize when iterator is in NON_INITIALIZED state

We hit a failed BT_ASSERT in the specific scenario explained below (and
illustrated by the new test).

The problem
-----------

Let's say we have a graph with this topology, where everything is
implemented in Python:

  MySink [in] <-> [out] MyFilter (MyFilterIter) [in] <-> [out] MySource (MySourceIter)

And the following sequence of events:

1. MySink's _user_graph_is_configured method creates an iterator on its
   "in" input port.  This runs MyFilterIter's __init__ method.
2. MyFilterIter's __init__ method starts by creating an upstream
   iterator on its "in" input port.  This iterator, of type
   MySourceIter, is initialized successfully.
3. MyFilterIter's __init__ method creates some other data structure which
   happens to form a Python reference cycle (the MyFilterIter instance has a
   reference on an object, which has a reference on the MyFilterIter
   instance).
4. MyFilterIter's __init__ method encounters an error, so an exception
   is raised.

When the MySourceIter is created, an entry is added to MyFilterIter's
upstream_msg_iters array (in the underlying
bt_self_component_port_input_message_iterator object).

When the exception is raised, because of the reference cycle, the
MyFilterIter Python object stays alive.  It has a reference on the
MySourceIter Python object, which keeps the underlying
bt_self_component_port_input_message_iterator object alive as well.

When the create_self_component_input_port_message_iterator call realizes
that the initialization of the filter iterator failed, it does a put_ref
on the iterator object, which ends up calling
bt_self_component_port_input_message_iterator_destroy.  In there, we
assert that:

  BT_ASSERT(iterator->upstream_msg_iters->len == 0);

This assertion does not hold, because the array still contains the entry
for the upstream iterator (on MySource).

Normally, the call to
bt_self_component_port_input_message_iterator_try_finalize, earlier in
bt_self_component_port_input_message_iterator_destroy, should take care
of unlinking any upstream or downstream iterator.  However, that step is
completely skipped because the iterator is still in the NON_INITIALIZED
state.

To reproduce, it is important to have the two conditions before the
exception is raised:

  - an upstream iterator is created: otherwise, the upstream_msg_iters
    array is empty so the assertion is true
  - a Python reference cycle exists, keeping the MyFilterIter object
    alive: otherwise, it is destroyed when the exception is raised (or
    caught?), which destroys the MySourceIter object, which destroys the
    underlying bt_self_component_port_input_message_iterator object (or
    the source iterator), which removes itself from the filter
    iterator's upstream_msg_iters array.  The array now being empty, the
    assertion would be true.

Note that the Python reference cycle is a user error that is not
desirable, but likely to happen.  To avoid any memory leak due to such a
reference cycle, I think the filter iterator here should normally make
sure to break the reference cycle.  In a successful execution, it would
be in _user_finalize.  If __init__ fails, it should make sure to not
leave a reference cycle in place, perhaps by using a try-except to clear
it before exiting the function.

The fix
-------

I believe it's wrong to skip the _try_finalize function even when we are
in this state, because it's possible (as shown above) for an iterator
initialize method to create some upstream iterators but then fail.  The
iterator remains in the NON_INITIALIZED state, but has some upstream
iterators.

I have changed
bt_self_component_port_input_message_iterator_try_finalize so that when
the iterator is in the NON_INITIALIZED, we do unlink the upstream and
downstream iterators.  However, I don't think we want to call the
iterator's finalize method in that case, since it hasn't been properly
initialized (a bit like a C++ object destructor is not called if the
constructor throws).  So I made it so the finalize method is only called
if the state is not NON_INITIALIZED.

Change-Id: Ibe2cbc729fc81ff0d68219400d925110242b7ae1
Signed-off-by: Simon Marchi <simon.marchi@efficios.com>
Reviewed-on: https://review.lttng.org/c/babeltrace/+/2633
Tested-by: jenkins <jenkins@lttng.org>
Reviewed-by: Francis Deslauriers <francis.deslauriers@efficios.com>