[ISSUE #42] Fix critical issues in the code #43
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR addresses critical issues identified in issue #42 by implementing comprehensive fixes across the codebase. The changes focus on improving Docker port management, test reliability, and code formatting while maintaining functionality.
- Replaces fixed port bindings with dynamic port allocation in Redis Testcontainers to prevent conflicts
- Adds conditional test execution using environment variables to make Redis tests optional
- Improves code formatting and error message clarity throughout the codebase
Reviewed Changes
Copilot reviewed 39 out of 39 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| support/src/testFixtures/kotlin/com/linecorp/cse/reqshield/support/redis/RedisContainer.kt | Replaces fixed port binding with dynamic port exposure for Redis container |
| support/src/testFixtures/kotlin/com/linecorp/cse/reqshield/support/redis/AbstractRedisTest.kt | Adds environment variable condition to control Redis test execution |
| Multiple test files | Adds @EnabledIfEnvironmentVariable annotations to make Redis integration tests conditional |
| Multiple core modules | Improves code formatting, error messages, and lock implementation fixes |
| README.md / AGENTS.md | Adds documentation for testing practices and repository guidelines |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| val scheduled: ScheduledFuture<*> = | ||
| executor.scheduleAtFixedRate({ | ||
| if (future.isDone) { | ||
| return@scheduleAtFixedRate | ||
| } | ||
| }, GET_CACHE_INTERVAL_MILLIS, TimeUnit.MILLISECONDS) | ||
|
|
||
| val scheduleFuture = schedule() | ||
| val funcResult = executeGetCacheFunction(cacheGetter, key) | ||
| if (funcResult != null) { | ||
| future.complete(funcResult.value) | ||
| return@scheduleAtFixedRate | ||
| } | ||
|
|
||
| if (counter.incrementAndGet() >= reqShieldConfig.maxAttemptGetCache) { | ||
| future.complete(executeCallable({ callable.call() }, false)) | ||
| } | ||
| }, GET_CACHE_INTERVAL_MILLIS, GET_CACHE_INTERVAL_MILLIS, TimeUnit.MILLISECONDS) | ||
|
|
||
| future.whenComplete { _, _ -> scheduleFuture.cancel(false) } | ||
| future.whenComplete { _, _ -> scheduled.cancel(false) } |
There was a problem hiding this comment.
The change from schedule() to scheduleAtFixedRate() creates a potential memory leak. If the future completes quickly, the scheduled task will continue running at fixed intervals indefinitely until manually cancelled, consuming unnecessary resources.
Copilot uses AI. Check for mistakes.
| val before = lockMap.size | ||
| lockMap.entries.removeIf { now > it.value.expiresAt } | ||
| val after = lockMap.size | ||
| if (log.isTraceEnabled && before > after) { | ||
| log.trace("Cleaned up {} expired locks, {} remaining", before - after, after) |
There was a problem hiding this comment.
The cleanup logic captures before and after sizes but only uses them for logging. Consider moving the size calculations inside the if (log.isTraceEnabled) block to avoid unnecessary operations when trace logging is disabled.
| val before = lockMap.size | |
| lockMap.entries.removeIf { now > it.value.expiresAt } | |
| val after = lockMap.size | |
| if (log.isTraceEnabled && before > after) { | |
| log.trace("Cleaned up {} expired locks, {} remaining", before - after, after) | |
| val removed = lockMap.entries.removeIf { now > it.value.expiresAt } | |
| if (log.isTraceEnabled) { | |
| val before = lockMap.size + (if (removed) 1 else 0) // Approximate, since removeIf returns true if any were removed | |
| val after = lockMap.size | |
| if (before > after) { | |
| log.trace("Cleaned up {} expired locks, {} remaining", before - after, after) | |
| } |
Copilot uses AI. Check for mistakes.
| return when (annotation.nullHandling) { | ||
| com.linecorp.cse.reqshield.spring.webflux.annotation.NullHandling.EMIT_EMPTY -> | ||
| resultMono.flatMap { value -> | ||
| if (value == null) { | ||
| Mono.empty() | ||
| } else { | ||
| Mono.just(value) | ||
| } | ||
| } | ||
| com.linecorp.cse.reqshield.spring.webflux.annotation.NullHandling.ERROR -> | ||
| resultMono.flatMap { value -> | ||
| if (value == null) { | ||
| Mono.error(IllegalStateException("ReqShieldCacheable returned null for key=$cacheKey")) | ||
| } else { | ||
| Mono.just(value) | ||
| } | ||
| } |
There was a problem hiding this comment.
The null handling logic is duplicated across both enum cases. Consider extracting this into a helper method or using switchIfEmpty() and handle() operators to reduce code duplication.
| return when (annotation.nullHandling) { | |
| com.linecorp.cse.reqshield.spring.webflux.annotation.NullHandling.EMIT_EMPTY -> | |
| resultMono.flatMap { value -> | |
| if (value == null) { | |
| Mono.empty() | |
| } else { | |
| Mono.just(value) | |
| } | |
| } | |
| com.linecorp.cse.reqshield.spring.webflux.annotation.NullHandling.ERROR -> | |
| resultMono.flatMap { value -> | |
| if (value == null) { | |
| Mono.error(IllegalStateException("ReqShieldCacheable returned null for key=$cacheKey")) | |
| } else { | |
| Mono.just(value) | |
| } | |
| } | |
| return resultMono.handle { value, sink -> | |
| when (annotation.nullHandling) { | |
| com.linecorp.cse.reqshield.spring.webflux.annotation.NullHandling.EMIT_EMPTY -> { | |
| if (value != null) sink.next(value) | |
| // else emit nothing (empty) | |
| } | |
| com.linecorp.cse.reqshield.spring.webflux.annotation.NullHandling.ERROR -> { | |
| if (value == null) sink.error(IllegalStateException("ReqShieldCacheable returned null for key=$cacheKey")) | |
| else sink.next(value) | |
| } | |
| } |
Copilot uses AI. Check for mistakes.
1 participant
No description provided.