mlevkov opened a new pull request, #4064:
URL: https://github.com/apache/iggy/pull/4064
Closes #4062.
## The leak
`SourceManager::start_connector` takes a fresh `plugin_id`, calls
`init_source`, and records that id on `SourceDetails` only after the handler
tasks are spawned. In between, the instance exists inside the plugin and
nothing outside it knows the id: `stop_connector` closes whatever
`details.info.id` holds, which is still the previous instance.
`setup_source_producer` returning early through `?` therefore stranded the new
one for the life of the process. `source::init` already cleaned up on the
identical failure, which is the asymmetry @hubcio pointed at.
For a plugin whose open only allocates, the orphan is wasted memory. For one
that takes a process-global resource it is a live fault: a shared listener
stays bound and answering into a queue nothing drains, and every retried
restart then fails on the identity the orphan never released.
## A guard, not a cleanup branch
This deviates from the fix in the review, which was to mirror
`source::init`'s error arm at the call site, so it is worth saying why rather
than leaving it to be found.
The window is defined by the two statements that open the instance and
record its id, not by which call between them happens to be fallible today. A
cleanup branch is correct only for the one `?` that exists now, and silently
wrong for the next one somebody adds. `SourceInstanceGuard` is armed at
`init_source` and disarmed once the id is recorded, so every path out of that
window closes the instance, including a panic.
It also made the behaviour testable. `Container<SourceApi>` only comes from
`dlopen`, so `start_connector` cannot be exercised in a unit test at all, while
a guard holding the bare `extern "C" fn` can be driven directly.
The close-and-report itself is now one function shared with `source::init`,
so the two sites cannot drift. `source::init` keeps its existing control flow;
only the duplicated body moved.
No `cleanup_sender` on this path: `spawn_source_handler` is what registers
the sender, and it has not run yet.
## Tests
Three, each mutation-checked, each mutant confirmed to compile first:
- an armed guard closes, and closes *its own* id, since closing another
would leave this instance open and tear down a live one
- a disarmed guard does not close, or a source that just started
successfully would be torn down
- a refused close (`-1`, the code the SDK returns for an unknown id) is
reported and not propagated, because unwinding out of `drop` would be worse
than the leak it is cleaning up after
Each test owns its stub and statics rather than sharing a pair, which would
have made two of them race in the same process.
## What is not covered, and why
**The guard's placement in `start_connector` has no test.** I verified that
rather than assuming it: disarming the guard immediately after construction
restores the original leak, compiles, and the suite still passes.
Reaching that path needs a real `Container`, so it cannot be a unit test,
and the only route into `start_connector` is `POST /sources/{key}/restart`.
Making `setup_source_producer` fail there means either a config the local
provider will serve on restart but not at boot, which today works only because
of the version selection in #3848 and would break when that is fixed, or
stopping the broker mid-test. Both couple this regression test to something
unrelated to it, so I left it out rather than write a test that fails for the
wrong reason later. Happy to add either if you would rather have the coverage
than the independence.
`source::init`'s cleanup remains covered only by `error_isolation.rs`
asserting the connector reports `Error`, which it did before this change too.
## Verification
`cargo fmt`, `cargo sort --no-format`, clippy at both feature sets, rustdoc
under `-D warnings`, 198 unit tests in `iggy-connectors`, and `stdout_sink` +
`random_source` still build.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
