Skip to content

Feature/destroy filter method - #133

Merged
FionaSprinkles merged 4 commits into
mainfrom
feature/destroy-filter-method
Feb 25, 2026
Merged

Feature/destroy filter method#133
FionaSprinkles merged 4 commits into
mainfrom
feature/destroy-filter-method

Conversation

@FionaSprinkles

@FionaSprinkles FionaSprinkles commented Feb 25, 2026

Copy link
Copy Markdown

This PR ensures that all registered filters are properly destroyed when the server shuts down.

Summary by CodeRabbit

  • New Features

    • Exposed a programmatic stop method for the processing pipeline so it can be cleanly terminated from outside.
  • Bug Fixes

    • Improved shutdown sequence: pipeline stop is invoked during shutdown, with clearer logging and guaranteed completion handling to ensure internal resources are cleaned up.

@FionaSprinkles FionaSprinkles linked an issue Feb 25, 2026 that may be closed by this pull request
@coderabbitai

coderabbitai Bot commented Feb 25, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 85a4134 and 50e8e01.

📒 Files selected for processing (1)
  • src/main/java/org/juv25d/App.java

📝 Walkthrough

Walkthrough

Adds a graceful shutdown flow: App's shutdown hook now calls pipeline.stop() (with try/catch/finally and extra log messages); Pipeline gains stop() which calls matcher.destroy(); FilterMatcher gains destroy() which calls registry.shutdown().

Changes

Cohort / File(s) Summary
Shutdown hook (App)
src/main/java/org/juv25d/App.java
Updated shutdown hook: now calls pipeline.stop() and wraps the call in try/catch/finally with additional log messages (including "Shutting down..." and "Shutdown hook finished").
Pipeline & Matcher lifecycle
src/main/java/org/juv25d/Pipeline.java, src/main/java/org/juv25d/filter/FilterMatcher.java
Added public void stop() to Pipeline delegating to matcher.destroy(), and added public void destroy() to FilterMatcher which calls registry.shutdown() to tear down resources.

Sequence Diagram

sequenceDiagram
    participant App as App
    participant Pipeline as Pipeline
    participant FilterMatcher as FilterMatcher
    participant Registry as Registry

    App->>Pipeline: stop()
    activate Pipeline
    Pipeline->>FilterMatcher: destroy()
    activate FilterMatcher
    FilterMatcher->>Registry: shutdown()
    activate Registry
    Registry-->>FilterMatcher: cleanup complete
    deactivate Registry
    FilterMatcher-->>Pipeline: destroy complete
    deactivate FilterMatcher
    Pipeline-->>App: stop complete
    deactivate Pipeline
    Note over App: Shutdown hook logs progress and completion
Loading

Estimated Code Review Effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 I pause my hops, I softly sigh,
Pipeline sleeps beneath the sky,
Matcher whispers, "Registry, close",
A tidy end before the doze,
Nibble of code, a graceful bows. 🌙✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Feature/destroy filter method' is partially related to the changeset. While a destroy method is added to FilterMatcher, the main objective is to ensure proper filter cleanup during server shutdown, which involves coordinating changes across App.java, Pipeline.java, and FilterMatcher.java.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feature/destroy-filter-method

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/main/java/org/juv25d/Pipeline.java (1)

26-28: Make stop() idempotent to harden shutdown.

If stop() is invoked multiple times, Line 27 will repeatedly call matcher.destroy(). Guarding this once avoids duplicate teardown side effects.

Proposed refactor
 package org.juv25d;
 
 import org.juv25d.filter.Filter;
 import org.juv25d.filter.FilterChainImpl;
 import org.juv25d.filter.FilterMatcher;
 import org.juv25d.http.HttpRequest;
 import org.juv25d.router.Router;
+import java.util.concurrent.atomic.AtomicBoolean;
 
 import java.util.List;
 
 public class Pipeline {
 
     private final FilterMatcher matcher;
     private final Router router;
+    private final AtomicBoolean stopped = new AtomicBoolean(false);
@@
     public void stop() {
-        matcher.destroy();
+        if (stopped.compareAndSet(false, true)) {
+            matcher.destroy();
+        }
     }
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/Pipeline.java` around lines 26 - 28, The stop()
method currently always calls matcher.destroy(), causing duplicate teardown if
invoked multiple times; make stop() idempotent by guarding the destroy
call—e.g., add a private boolean/AtomicBoolean (e.g., "stopped" or "destroyed")
or set matcher to null after destroying and check that flag/null before calling
matcher.destroy() in stop(), and ensure the check-and-destroy is thread-safe
(synchronize the method or use compare-and-set on an AtomicBoolean) so
matcher.destroy() runs only once.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/main/java/org/juv25d/App.java`:
- Around line 35-37: Wrap the shutdown hook teardown so exceptions from
pipeline.stop() don't prevent completion logging: enclose the pipeline.stop()
call inside a try/catch and log any thrown exception (include the exception via
logger.error) and put logger.info("Server shutting down...") in a finally block
to guarantee it always runs; locate the shutdown hook code that calls
pipeline.stop() and logger.info in App.java and update it accordingly.

---

Nitpick comments:
In `@src/main/java/org/juv25d/Pipeline.java`:
- Around line 26-28: The stop() method currently always calls matcher.destroy(),
causing duplicate teardown if invoked multiple times; make stop() idempotent by
guarding the destroy call—e.g., add a private boolean/AtomicBoolean (e.g.,
"stopped" or "destroyed") or set matcher to null after destroying and check that
flag/null before calling matcher.destroy() in stop(), and ensure the
check-and-destroy is thread-safe (synchronize the method or use compare-and-set
on an AtomicBoolean) so matcher.destroy() runs only once.

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ad74e89 and 85a4134.

📒 Files selected for processing (3)
  • src/main/java/org/juv25d/App.java
  • src/main/java/org/juv25d/Pipeline.java
  • src/main/java/org/juv25d/filter/FilterMatcher.java

Comment thread src/main/java/org/juv25d/App.java Outdated

@kristinaxm kristinaxm left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving :)

@Tyreviel Tyreviel left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@FionaSprinkles
FionaSprinkles merged commit 93fd9aa into main Feb 25, 2026
2 checks passed
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.

Destroy filter method

3 participants