Timeoutfilter#151
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces a TimeoutFilter that enforces configurable request timeout (default 2000 ms) using a thread pool executor with 504 Gateway Timeout response on expiry, and a SlowPlugin test handler that suspends for 5 seconds, registered in the router configuration. Changes
Sequence DiagramsequenceDiagram
participant Client
participant TimeoutFilter as TimeoutFilter
participant Executor as Thread Pool Executor
participant Handler as Request Handler
participant Response
Client->>TimeoutFilter: HTTP Request
TimeoutFilter->>TimeoutFilter: Log request start
TimeoutFilter->>Executor: Submit doFilter task
Executor->>Handler: Execute in thread pool
alt Completes within timeout
Handler->>Handler: Process request
Handler->>Response: Generate response
Response->>TimeoutFilter: Return result
TimeoutFilter->>TimeoutFilter: Log completion
TimeoutFilter->>Client: HTTP 200/Success
else Timeout occurs
Executor->>Executor: Task exceeds 2000ms
Executor->>TimeoutFilter: Timeout exception
TimeoutFilter->>Handler: Cancel task
TimeoutFilter->>TimeoutFilter: Log timeout event
TimeoutFilter->>Client: HTTP 504 Gateway Timeout
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/main/java/org/juv25d/plugin/SlowPlugin.java (1)
13-17: Consider restoring the interrupt flag before returning.When catching
InterruptedException, best practice is to restore the interrupt status so that calling code can detect the interruption. While returning immediately here is functionally fine for this demo plugin, preserving the flag improves robustness.♻️ Suggested improvement
try { Thread.sleep(5000); } catch (InterruptedException e) { + Thread.currentThread().interrupt(); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/plugin/SlowPlugin.java` around lines 13 - 17, The catch block swallowing InterruptedException in SlowPlugin should restore the thread's interrupt status before returning; update the catch for InterruptedException in the Thread.sleep(5000) handling inside class SlowPlugin to call Thread.currentThread().interrupt() (or equivalent) prior to the return so the interrupt flag is preserved for calling code.src/main/java/org/juv25d/filter/TimeoutFilter.java (1)
51-55: Consider unwrapping ExecutionException and preserving InterruptedException.The generic catch for
Exceptionlumps togetherExecutionException,CancellationException, andInterruptedException. For better diagnostics:
- Unwrap
ExecutionExceptionto expose the root cause.- Restore interrupt flag if
InterruptedExceptionis caught.♻️ More explicit exception handling
- } catch (Exception e) { - - future.cancel(true); - throw new RuntimeException(e); + } catch (ExecutionException e) { + future.cancel(true); + Throwable cause = e.getCause(); + if (cause instanceof IOException) { + throw (IOException) cause; + } + throw new RuntimeException(cause != null ? cause : e); + } catch (InterruptedException e) { + future.cancel(true); + Thread.currentThread().interrupt(); + throw new RuntimeException("Request processing interrupted", e); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/filter/TimeoutFilter.java` around lines 51 - 55, In TimeoutFilter replace the broad catch (Exception e) around the future.get() / future.cancel(true) logic with explicit handling: catch ExecutionException and rethrow a RuntimeException (or propagate) using e.getCause() so the root cause is preserved; catch InterruptedException, restore the interrupted flag via Thread.currentThread().interrupt() and then throw or wrap that exception appropriately; handle CancellationException separately if needed (rethrow or wrap). Keep the existing future.cancel(true) behavior but ensure these specific catches are used instead of a generic Exception to preserve diagnostics and interruption semantics.
🤖 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/filter/TimeoutFilter.java`:
- Line 15: Replace the hardcoded static TIMEOUT_MS in TimeoutFilter with a
configurable value: change TIMEOUT_MS from a private static final to an instance
field on class TimeoutFilter, add a constructor (or setter) that accepts a long
timeoutMs (or read from ConfigLoader/System property/env like ServerLogging
does) and use that field in the existing timeout logic; ensure a sensible
default (2000) if config is missing and update any instantiation sites to pass
the configured value or supply the ConfigLoader/System lookup inside the
constructor.
- Around line 29-49: The TimeoutFilter currently cancels the worker Future while
both the worker thread (in executor submitting chain.doFilter(req, res)) and the
timeout handler write to the shared HttpResponse (res), causing a race; change
the flow to have the worker use a dedicated response container (e.g., create a
local ResponseWrapper or MutableResponse instance passed to chain.doFilter in
place of res inside the submitted task) and only copy the wrapper's status,
headers, and body into the real res after future.get(...) returns successfully
(before logging COMPLETED); on TimeoutException call future.cancel(true) and
then set the 504 response on the real res without risking concurrent writes;
update TimeoutFilter to reference the new wrapper class and ensure the copying
logic is atomic (synchronized block or single-threaded handoff) so the shared
res is never modified concurrently.
- Around line 17-18: The static ExecutorService named executor in class
TimeoutFilter is never shut down; register a JVM shutdown hook in the
TimeoutFilter class (e.g., in a static initializer) that gracefully shuts down
the executor (call shutdown(), awaitTermination with a timeout, then
shutdownNow() if needed) and log/handle interrupted exceptions so threads are
cleaned up on application exit; ensure the hook references the same executor
variable so the service is terminated when the JVM shuts down.
---
Nitpick comments:
In `@src/main/java/org/juv25d/filter/TimeoutFilter.java`:
- Around line 51-55: In TimeoutFilter replace the broad catch (Exception e)
around the future.get() / future.cancel(true) logic with explicit handling:
catch ExecutionException and rethrow a RuntimeException (or propagate) using
e.getCause() so the root cause is preserved; catch InterruptedException, restore
the interrupted flag via Thread.currentThread().interrupt() and then throw or
wrap that exception appropriately; handle CancellationException separately if
needed (rethrow or wrap). Keep the existing future.cancel(true) behavior but
ensure these specific catches are used instead of a generic Exception to
preserve diagnostics and interruption semantics.
In `@src/main/java/org/juv25d/plugin/SlowPlugin.java`:
- Around line 13-17: The catch block swallowing InterruptedException in
SlowPlugin should restore the thread's interrupt status before returning; update
the catch for InterruptedException in the Thread.sleep(5000) handling inside
class SlowPlugin to call Thread.currentThread().interrupt() (or equivalent)
prior to the return so the interrupt flag is preserved for calling code.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/org/juv25d/filter/TimeoutFilter.javasrc/main/java/org/juv25d/plugin/SlowPlugin.javasrc/main/java/org/juv25d/router/RouterConfig.java
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/org/juv25d/filter/TimeoutFilter.java (1)
56-60: Consider distinguishing exception types for clearer error handling.The current catch-all
Exceptionhandles bothInterruptedException(current thread interrupted) andExecutionException(downstream handler failed). ForExecutionException, you could unwrap and rethrow the original cause to preserve the stack trace. ForInterruptedException, consider re-interrupting the thread.♻️ Optional: More precise exception handling
- } catch (Exception e) { - - future.cancel(true); - throw new RuntimeException(e); + } catch (InterruptedException e) { + future.cancel(true); + Thread.currentThread().interrupt(); + throw new RuntimeException("Request processing interrupted", e); + } catch (ExecutionException e) { + future.cancel(true); + Throwable cause = e.getCause(); + if (cause instanceof IOException ioe) { + throw ioe; + } + throw new RuntimeException(cause); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/filter/TimeoutFilter.java` around lines 56 - 60, The catch-all in TimeoutFilter around the future handling should be split: catch InterruptedException to call future.cancel(true), re-interrupt the thread (Thread.currentThread().interrupt()) and throw a RuntimeException wrapping the InterruptedException; catch ExecutionException to call future.cancel(true) and unwrap/throw the original cause (e.getCause()) wrapped in a RuntimeException (or rethrow if it's already a RuntimeException/Error) so the original stack trace is preserved; ensure future.cancel(true) still runs for both paths (or move it to a finally block around the existing future.get() call) and reference TimeoutFilter and the block where future.cancel(true) is currently invoked.
🤖 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/util/ConfigLoader.java`:
- Around line 189-191: The getTimeoutMs(int i) method currently ignores its
parameter and always returns 0; update ConfigLoader by adding a timeoutMs field
(e.g., private long timeoutMs), parse that value from the YAML in
loadConfiguration() under the server config (with a sensible default if absent),
and change getTimeoutMs(int i) to return timeoutMs if set (or the provided
default parameter i when not configured). Ensure the YAML parsing uses the same
key name you add and that TimeoutFilter will call getTimeoutMs(2000) to receive
the configured timeout or the 2000ms fallback.
---
Nitpick comments:
In `@src/main/java/org/juv25d/filter/TimeoutFilter.java`:
- Around line 56-60: The catch-all in TimeoutFilter around the future handling
should be split: catch InterruptedException to call future.cancel(true),
re-interrupt the thread (Thread.currentThread().interrupt()) and throw a
RuntimeException wrapping the InterruptedException; catch ExecutionException to
call future.cancel(true) and unwrap/throw the original cause (e.getCause())
wrapped in a RuntimeException (or rethrow if it's already a
RuntimeException/Error) so the original stack trace is preserved; ensure
future.cancel(true) still runs for both paths (or move it to a finally block
around the existing future.get() call) and reference TimeoutFilter and the block
where future.cancel(true) is currently invoked.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/juv25d/filter/TimeoutFilter.javasrc/main/java/org/juv25d/util/ConfigLoader.java
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/main/java/org/juv25d/filter/TimeoutFilter.java (1)
39-65:⚠️ Potential issue | 🟠 MajorRace condition on shared
HttpResponseremains unaddressed.After timeout,
future.cancel(true)is asynchronous — the worker thread may still be executingchain.doFilter(req, res)and modifyingreswhile lines 57-59 also write tores. This can corrupt response state.Additionally, the exception handling at lines 61-64 results in triple-wrapped exceptions:
IOException→RuntimeException(line 43) →ExecutionException→RuntimeException(line 64). Since the method signature declaresthrows IOException, consider unwrapping to preserve the original exception.,
🔧 Suggested fix for exception unwrapping
} catch (Exception e) { - future.cancel(true); - throw new RuntimeException(e); + Throwable cause = e; + if (e instanceof ExecutionException && e.getCause() != null) { + cause = e.getCause(); + } + if (cause instanceof RuntimeException && cause.getCause() instanceof IOException) { + throw (IOException) cause.getCause(); + } + throw new RuntimeException(cause); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/filter/TimeoutFilter.java` around lines 39 - 65, TimeoutFilter currently cancels the worker Future asynchronously which can race with the worker still mutating the shared HttpResponse (res); wrap or replace the mutable response with a thread-safe wrapper (e.g., a ResponseWrapper that atomically tracks committed state using an AtomicBoolean and ignores further writes once timed out) and pass that wrapper to chain.doFilter(req, wrappedRes) so the timeout handler can safely set 504 without corruption; additionally, change the catch(Exception e) block to unwrap ExecutionException (check e.getCause()) and rethrow the original IOException if present (since the method declares throws IOException) otherwise rethrow a RuntimeException with the underlying cause so we don't triple-wrap the original exception.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main/java/org/juv25d/filter/TimeoutFilter.java`:
- Around line 39-65: TimeoutFilter currently cancels the worker Future
asynchronously which can race with the worker still mutating the shared
HttpResponse (res); wrap or replace the mutable response with a thread-safe
wrapper (e.g., a ResponseWrapper that atomically tracks committed state using an
AtomicBoolean and ignores further writes once timed out) and pass that wrapper
to chain.doFilter(req, wrappedRes) so the timeout handler can safely set 504
without corruption; additionally, change the catch(Exception e) block to unwrap
ExecutionException (check e.getCause()) and rethrow the original IOException if
present (since the method declares throws IOException) otherwise rethrow a
RuntimeException with the underlying cause so we don't triple-wrap the original
exception.
OskarLundqvist33
previously approved these changes
Feb 28, 2026
OskarLundqvist33
left a comment
OskarLundqvist33
left a comment
There was a problem hiding this comment.
Cancelling with future.cancel(true):
Cancelling a thread with true only interrupts it. That only works if the code being executed actually checks interruption (e.g. in blocking methods like Thread.sleep or explicit Thread.interrupted() checks). In plugins/handlers, chain.doFilter(...) may not always support interruption. If so, cancellation might not actually stop execution. Cancelling the future with true relies on the handler code responding to interrupts. If downstream handlers don’t honor interruption, they may continue running after timeout. Consider documenting this limitation or enforcing cooperative interruption. This may not need a change but could be good to have documented so the issue will not pop up later, could also be set up as a different issue forcing so that it will not occur in the future.
Otherwise great work! It looks clean and seems to work great!
Johan-jans
previously approved these changes
Mar 1, 2026
Johan-jans
dismissed stale reviews from OskarLundqvist33 and themself
via
March 1, 2026 19:39
be07d9a
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/test/java/org/juv25d/filter/TimeoutFilterTest.java`:
- Around line 68-70: The test currently asserts any RuntimeException from
filter.doFilter(req, res, chain); tighten it to assert that the thrown
RuntimeException wraps an IOException cause. Update the assertion in
TimeoutFilterTest so that the thrown exception
isInstanceOf(RuntimeException.class) and hasCauseInstanceOf(IOException.class)
(or extract the cause and assert it is an IOException) when calling
filter.doFilter(req, res, chain), referencing the filter.doFilter invocation and
the expected IOException cause.
- Around line 39-58: The test slowRequest_sets504 currently only asserts the 504
response but must also assert that downstream execution was
cancelled/interrupted; modify the doAnswer for chain.doFilter in
TimeoutFilterTest so it captures the executing thread and records
interruption/cancellation (e.g., set an AtomicBoolean or CountDownLatch when
Thread.currentThread().isInterrupted() or when InterruptedException is caught)
while still simulating sleep, then after calling TimeoutFilter.doFilter(req,
res, chain) assert that the recorded flag indicates the downstream thread was
interrupted/cancelled; reference TimeoutFilterTest::slowRequest_sets504,
TimeoutFilter, and chain.doFilter to locate and update the test.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/filter/TimeoutFilter.java`:
- Around line 61-65: The catch-all in TimeoutFilter around future.get() swallows
InterruptedException without restoring the thread interrupt flag; update the
try/catch to handle InterruptedException separately (e.g., catch
InterruptedException ie) where you call future.cancel(true), call
Thread.currentThread().interrupt(), and then rethrow or wrap the exception, and
keep a separate catch for ExecutionException/Exception to handle task failures;
refer to the existing future variable and the enclosing method in TimeoutFilter
when making the change.
EmmaTravljanin
force-pushed
the
Timeout-filter-branch
branch
from
March 1, 2026 20:23
9fbb573 to
53b5985
Compare
fmazmz
approved these changes
Mar 1, 2026
kristinaxm
approved these changes
Mar 1, 2026
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
5 participants
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.
Co-authored by: johan.jansson@iths.se
Closes #70
Summary by CodeRabbit