Skip to content

End the MySQL thread on every poll_host exit - #595

Closed
somethingwithproof wants to merge 5 commits into
Cacti:developfrom
somethingwithproof:fix/poll-host-teardown
Closed

End the MySQL thread on every poll_host exit#595
somethingwithproof wants to merge 5 commits into
Cacti:developfrom
somethingwithproof:fix/poll-host-teardown

Conversation

@somethingwithproof

Copy link
Copy Markdown
Member

Closes #594. Stacked on #589; merge that first.

poll_host() leaves through three places. Two called mysql_thread_end(), one did not:

poller.c:810   mysql_thread_end();     /* set.ping_only exit */
poller.c:1962  mysql_thread_end();     /* normal exit        */
poller.c:523   return;                 /* device row missing */

There is no mysql_thread_init() in the tree, so the client library sets its thread-local state up on first use and mysql_thread_end() is what releases it. poll_host() has already issued db_query() before it reaches that branch, so the state exists and is then abandoned. Spine is thread-per-device, so it is once per affected device per cycle.

The branch fires when SELECT ... FROM host WHERE id = %i AND deleted = '' does not return exactly one row: a device deleted or soft-deleted between the poller picking it up and the thread reading it, or a remote poller whose host table has not caught up.

Why it happened

The teardown was written out three times, six SPINE_FREE calls plus the same fourteen-line connection release. The three copies were not in the same order and, as here, did not contain the same steps. Nothing put them side by side.

The change

poll_host_release() for the two early exits, taking pointers so the caller's pointers are nulled rather than left dangling.

The normal exit could not use it: it calls buffer_output_errors(error_string, ...) after the point where the helper frees error_string. Rather than reorder that, it shares poll_host_release_connections() only and keeps the rest of its sequence. So the fourteen-line connection block is now in one place across all three, and the six frees are shared across the two that can share them.

poll_host() drops from 1,620 to 1,570 lines.

Verification

This one is not unit-testable: mysql_thread_end() is a libmysql call with no observable return, and reaching the branch needs a device row to vanish between two queries. Saying so rather than dressing up a weaker test as proof.

What was checked: every return in poll_host(), and its fall-off-the-end exit, now reaches mysql_thread_end(). Clean ubuntu:24.04 build matching ci.yml, same four warnings as develop, make check green on all four binaries, 49 of 49 tests.

No CHANGELOG entry here; #578 is the changelog PR for this batch.

poll_host() is 1,926 lines and builds the same six queries twice, once for the
main poller and once for a remote one. The bodies differ by 33 of 141 lines,
and every difference is the same rule: the main poller reads items that are not
deleted, a remote poller reads the items assigned to it.

Nothing in that construction was reachable from a test, so a column added to
one copy and not the other would not have been caught.

Extract the rule as poller_item_scope() and poller_owner_scope(), covered by
five cases in test_linked against the shipped poller.o: the deleted filter on
the main poller, the ownership filter on a remote one, the empty fragment the
main poller needs so callers can interpolate unconditionally, and a degenerate
buffer.

No call site changes yet. Collapsing the two branches onto these helpers is the
next step and is worth its own review, because the shipped query1 spells its
tail 'poller_id=%i' while the others spell it 'poller_id = %i', so the unified
text will normalise that.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
The two branches drifted: the remote copy never picked up the dbonupdate
handling the main copy grew, and nothing could have caught that because the
construction was not reachable from a test.

Captured what both branches emit across the vectors they switch on, collapsed
them onto poller_item_scope()/poller_owner_scope(), and diffed. Output is
byte-identical except query1 on a remote poller, which now spells its tail
'poller_id = N' rather than 'poller_id=N' to match the other five queries.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
set.dbonupdate is 1 on MySQL 8, which deprecated VALUES() in ON DUPLICATE KEY
UPDATE. The main poller switched to the row-alias form; the remote branch kept
the deprecated one because it had its own copy of the suffix. Closes Cacti#590.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
The construction was 167 lines in the middle of a 1,795-line function, so
nothing could reach it. It now takes its inputs as arguments and fills a
struct, which is what lets the golden capture become a test that runs instead
of a file with instructions attached.

poll_host is 1,620 lines, down from 1,927 on develop.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
poll_host() leaves through three places and each spelled its teardown out
again. The copies were not in the same order and did not hold the same steps:
the device-row-missing path never called mysql_thread_end(), so a device
deleted mid-cycle leaked the client library's thread-local state once per
cycle. Closes Cacti#594.

The two early exits now share poll_host_release(). The normal exit uses
error_string after the point where that helper frees it, so it shares only
poll_host_release_connections() and keeps its own ordering.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
@somethingwithproof

Copy link
Copy Markdown
Member Author

Consolidated into #597, which carries this branch's commits unchanged.

Every pair of these ten conflicted on tests/unit/test_linked.c because each appended to the same registration array, so whichever merged first would have forced a rebase on the other nine. #597 is one review and one approval for the same 20 commits, still one logical change each.

Nothing here is dropped. Reopen this if you would rather review it separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

poll_host() skips mysql_thread_end() when the device row is missing

1 participant