Call NFPlugin.cleanup() at the end of the meter workflow - #243
Open
drnpkr wants to merge 1 commit into
Open
Conversation
NFPlugin.cleanup() is documented ('Method called for plugin cleanup') but
was never actually invoked anywhere in the package. Plugins that buffer
state across flows (e.g. time-window aggregators) therefore had no hook to
flush their final entries, silently losing them at end of capture.
Invoke udp.cleanup() for each plugin in meter_workflow, right after
meter_cleanup() expires the remaining cached flows (so all on_expire hooks
have already run) and before signalling completion.
Add test_plugin_cleanup: asserts cleanup() runs after all flows expire.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
NFPlugin.cleanup()is documented ("Method called for plugin cleanup") but is never actually invoked anywhere in the package. Plugins that buffer state across flows — time-window/bin aggregators, file writers, batched exporters — have no hook to flush their final buffered state, so that data is silently lost at the end of a capture.Fix
Invoke
udp.cleanup()for each plugin inmeter_workflow, right aftermeter_cleanup()has expired the remaining cached flows (so everyon_expirehook has already run) and before the meter signals completion viachannel.put(None).Test
Adds
test_plugin_cleanup: a plugin counts itson_expirecalls and writes the total fromcleanup(). The test asserts the marker file exists (i.e.cleanup()was actually called) and that the count equals the number of flows (i.e. it ran after all flows expired).Scope note — live captures (follow-up needed)
This fix covers the graceful end-of-capture path, which today is only reached when
capture_nexthits end-of-file — i.e. file sources. On live interfaces the meter loop has no EOF: stopping viaCtrl-C(or reachingmax_nflows, which internally raisesKeyboardInterrupt) makes the streamer callmeters[i].terminate()— a hard kill. In that path neithermeter_cleanup()(so noon_expirefor still-cached flows — a pre-existing loss unrelated to this PR) nor the newcleanup()hook runs.A proper live-source fix is a larger coordination change (meters ignoring
SIGINT, a shared stop event checked in the capture loop, and the streamer draining the channel until each meter sends itsNonesentinel instead of terminating). Deliberately kept out of this PR to keep it a minimal, reviewable fix; noted here so it isn't forgotten — follow-up PR planned.