app_chanspy+cel: Release channel iterator before chanspying

[wdoekes]

(This PR contains 4 commits, where the first three are really just refactoring. Only the last should change behaviour.)

Refactor channel spying so it never holds on to a channel iterator. Instead, we recreate the iterator when needed and skip channels that we've seen already, creating the illusion of using an iterator.

This change was needed because the iterator caused the yet-unseen channels in the iterator to be referenced by the iterator. This reference ensured that the channel does not get destroyed. (Which is good, because the iterator needs valid channels to work on.)

But, hanging on to a channel reference for longer than a short while conflicts with CEL logging. The CEL hangup logging is activated by the destruction of the channel. During chanspy activity, a bunch of channels would stay in limbo. First when the chanspying was done would those channels get their CEL hangup event logged.

The fix here is to hang on to channel iterators for only a short while. An alternative fix that makes CEL hangup logging independent of channel destruction was deemed more invasive.

This patch makes chanspy channel selection slightly more resource intensive. But that's a small price to pay for correct CEL hangup logging.

Fixes: #68

[wdoekes]

cherry-pick-to: 22
cherry-pick-to: 21
cherry-pick-to: 20

[wdoekes]

• As discussed on IRC, I want the commits applied without squashing (they all compile and run fine if merged in sequence);
• AsteriskGateTestMatrix (18, pjs2) failed, but that looks unrelated.
• I did not add any UserNote: stuff yet. I could add something like:

  CEL hangup logging is not longer delayed by concurrent ChanSpy activity.
  

• As for the fix itself (the last commit): I'm open to alternative solutions. I mention memory hoarding. This could be fixed by removing items from the vector that are not in the iterator. But then we'd want to switch to a linkedlist/hashmap instead.

[jcolp]

So I was pondering this some. ChanSpy is from a time when the only way to know about a channel was to get the channel, or list of channels, and go through them. Since then we've added channel snapshots and have a cache of them accessible using ast_channel_cache_by_name() which doesn't require accessing the channels or channel list at all. I think this should be examined as an alternative instead, so that holding references to channels is kept to a minimum - specifically when a channel is being spied on.

Anyone else have any thoughts on that idea?

[wdoekes]

@jcolp: Do you have a quick example of how to use those snapshots? Some bigger refactoring might be worth it.

We've been running this on prod now, but we did run into 2 similar deadlocks. I cannot prove that this patch caused it, but cannot disprove it either.

The two deadlocks occurred with two threads in:
got_optimized_out -> try_swap_optimize_out -> bridge_do_move -> bridge_complete_join -> bridge_channel_complete_join -> simple_bridge_join -> ast_channel_request_stream_topology_change -> simple_bridge_join -> ast_channel_request_stream_topology_change -> unreal_colp_stream_topology_request_change -> ast_unreal_lock_all
According to the core dumps we collected, they were stuck at ast_channel_lock_both(p->chan, p->owner). Both threads were supposedly already owner of one of the locks. Either the core dump misinformed, or that shouldn't happen. (Especially the thread that was holding on to p->owner but not p->chan, because it causes a locking inversion.)

If this changeset is to blame, I would put my money on some ast_autochan behaviour (swapping (locked?) channels?), but I don't really see how I changed anything in this respect... especially since there was only one ChanSpy invocation in one of these Asterisk runs, and it was 13 hours prior to the deadlock.

[jcolp]

I'm not sure what example would be applicable for this exactly, but "core show channels" uses the channel snapshot cache and snapshots. It doesn't pull from the channels container.

[sangoma-oss-cla]

All committers have signed the CLA.