Conversation
|
Warning Rate limit exceeded@jeme has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 44 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe changes introduce enhanced diagnostics, synchronization, and resource management across the index writer and lease management subsystems. New methods for locking and unlocking writer creation were added, lease recall now returns recalled items, and additional debug output and stack traces assist in tracing disposal and commit operations. Test code was updated for improved timing and logging. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as JsonIndexManagerTest
participant Manager as IndexWriterManager
participant Proxy as IndexWriterSafeProxy
participant LeaseMgr as LeaseManager
participant Storage as IJsonIndexStorageManager
Test->>Manager: Commit()
Manager->>Proxy: Commit()
Proxy->>Proxy: Log commit attempt (with id)
Proxy->>Proxy: Try inner commit
alt ObjectDisposedException
Proxy->>Proxy: Log disposal stack trace
Proxy->>Test: Rethrow exception
end
Test->>Manager: TakeSnapshotAsync / ResetIndexAsync
Manager->>LeaseMgr: RecallAll()
LeaseMgr->>Test: Return recalled leases
Test->>Storage: Delete()
Storage->>Manager: Lock()
Storage->>LeaseMgr: RecallAll()
Storage->>Manager: Close()
Storage->>provider: Delete()
Storage->>Manager: Unlock()
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
# Conflicts: # src/DotJEM.Json.Index2/Storage/IIndexStorageProvider.cs
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
src/DotJEM.Json.Index2.Management.Test/JsonIndexManagerTest.cs (1)
41-58:⚠️ Potential issuePotential data race on
disposedEvent/exceptionEventBoth variables are mutated from the
InfoStreamsubscription (likely on a different thread) and read from the test thread without any synchronisation.
This may lead to torn reads or missed writes.
Mark the fieldsvolatileor use anInterlocked.Exchange/Interlocked.CompareExchangepattern to avoid the race:-InfoStreamExceptionEvent? disposedEvent = null; -InfoStreamExceptionEvent? exceptionEvent = null; +volatile InfoStreamExceptionEvent? disposedEvent = null; +volatile InfoStreamExceptionEvent? exceptionEvent = null;
♻️ Duplicate comments (1)
src/DotJEM.Json.Index2/Leases/LeaseManager.cs (1)
194-203: Same missing-lock issue inTimeLimitedLease.WithLockReplicate the locking fix suggested for
Lease.
Otherwise concurrent access duringTerminate()can expose partially-disposed state.
🧹 Nitpick comments (11)
src/DotJEM.Json.Index2.Management.Test/JsonIndexManagerTest.cs (3)
24-25: Consider tightening (or parameterising) the MaxTime attribute
MaxTime(1000*60*12)allows the test to run for up to 12 minutes.
Even for an[Explicit]test this is unusually long and can mask hangs.
If the long timeout is required, consider extracting it to a constant / config so it is easy to adjust (e.g. when running in CI vs. locally).
65-75: Long-running busy-loop can slow the test suiteEven reduced to 1 minute, this loop can still dominate test execution time in CI.
Consider:
• lowering the duration further,
• using a cancellation token that is signalled when either event occurs, or
• parametrising the duration so CI can run a shorter version.
80-88: Catch block only logse.MessageImportant diagnostic information (stack trace, inner exceptions) is discarded. Replace with
Debug.WriteLine(e)orDebug.WriteLine(e.ToString())to preserve full context.-Console.WriteLine(e.Message); +Debug.WriteLine(e);src/DotJEM.Json.Index2/Storage/IIndexStorageProvider.cs (1)
11-14:Pathproperty name may collide withSystem.IO.Pathin call-sitesAlthough legal, using the identifier
Pathon an interface that is widely consumed can cause readability issues when mixed withusing System.IO;.
Consider renaming to something more specific likeStoragePathto avoid ambiguity.src/DotJEM.Json.Index2/IO/JsonIndexWriterManager.cs (2)
76-82: Re-entrant lock is fine, but redundant
Lease()acquireswriterPadLockand then callsWriterwhich will try to acquire the same lock. WhileMonitoris re-entrant, it incurs extra overhead.
You can callWriteroutside the lock and only lock when interacting withleaseManager.
112-115:writersdiagnostic may under-report live writersBecause dead references accumulate,
writers.Countgrows unbounded and the “alive” count diverges.
After counting, prune the list:lock (writersPadlock) { writers.RemoveAll(w => !w.TryGetTarget(out _)); }src/DotJEM.Json.Index2/IO/IndexWriterSafeProxy.cs (3)
15-17: Thread-safety ofwhoDisposedis not guaranteed
whoDisposedis assigned inDispose()/Dispose(bool)and read back fromCommit().
IfCommit()races withDispose()from another thread, there is a small window whereDispose()has not yet assigned the field, causing anullprintout and losing the diagnostic value. A simplevolatiledeclaration (or anInterlocked.Exchange) makes the intent explicit and prevents re-ordering.- private StackTrace whoDisposed; + private volatile StackTrace? whoDisposed;
37-46: Inconsistent logging API & stack-trace loss
Dispose…usesDebug.WriteLinewhileCommit()usesConsole.WriteLine.
Mixing sinks complicates log collection, especially in library code where consumers cannot easily redirectConsole.
I recommend routing everything through the same mechanism (e.g.Debug, or the project’s logger), and emitting the stack-trace directly instead of relying onToString()(which omits file/line information in Release builds).
199-217: Avoid swallowing context when re-throwingInside the
catch (ObjectDisposedException e)block you print diagnostics and thenthrow;.
That preserves the original stack, which is good, but all the extra information you just printed is lost for callers who rely on exception data (e.g. telemetry pipelines that do not capturestdout).
Consider attaching the disposal stacktrace to the exceptionDatadictionary before re-throwing; no API breakage and the info stays with the exception.catch (ObjectDisposedException e) { Console.WriteLine("Who disposed me:"); Console.WriteLine(whoDisposed); … - throw; + e.Data["DisposalStack"] = whoDisposed; + e.Data["CommitCallerStack"] = new StackTrace(); + throw; }src/DotJEM.Json.Index2/Leases/LeaseManager.cs (2)
45-53: Potential race condition when copying leasesYou clear
leasesinside the lock and then iteratecopyoutside.
OnReturned(invoked byTerminate()) will attempt toRemovethe same lease from the now-empty list.
While this is functionally harmless, it still acquires the lock for every lease and performs a linear scan on an empty list.
Consider removing the callback invocation or guardingOnReturnedwith a flag to skip removal when the manager has already recalled everything.
132-137:returned.Wait(500)can block forever on lost signalIf the consumer never calls
Dispose(), theManualResetEventSlimis never set andTerminate()waits the full 500 ms for each lease, turningRecallAll()into an O(n) delay.
Either:
- Use a much shorter timeout (e.g. 10 ms) and loop a few times, or
- Skip waiting when nothing is listening.
This avoids holding the manager hostage when mis-behaving clients forget to dispose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/DotJEM.Json.Index2.Management.Test/JsonIndexManagerTest.cs(6 hunks)src/DotJEM.Json.Index2.Management/Writer/IJsonIndexWriter.cs(1 hunks)src/DotJEM.Json.Index2.Test/JsonIndexTest.cs(1 hunks)src/DotJEM.Json.Index2/IO/IndexWriterSafeProxy.cs(4 hunks)src/DotJEM.Json.Index2/IO/JsonIndexWriterManager.cs(6 hunks)src/DotJEM.Json.Index2/Leases/Lease.cs(1 hunks)src/DotJEM.Json.Index2/Leases/LeaseManager.cs(7 hunks)src/DotJEM.Json.Index2/Storage/IIndexStorageProvider.cs(2 hunks)src/DotJEM.Json.Index2/Storage/IJsonIndexStorageManager.cs(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/DotJEM.Json.Index2/Leases/Lease.cs (1)
src/DotJEM.Json.Index2/Leases/LeaseManager.cs (4)
WithLock(121-124)WithLock(199-202)TOut(116-119)TOut(194-197)
src/DotJEM.Json.Index2/Storage/IJsonIndexStorageManager.cs (2)
src/DotJEM.Json.Index2/IO/JsonIndexWriterManager.cs (6)
Lock(19-19)Lock(42-45)Close(17-17)Close(93-115)Unlock(20-20)Unlock(46-49)src/DotJEM.Json.Index2/Storage/IIndexStorageProvider.cs (3)
Delete(13-13)Delete(21-24)Delete(39-56)
src/DotJEM.Json.Index2/Storage/IIndexStorageProvider.cs (3)
src/DotJEM.Json.Index2.Contexts/Storage/IStorageProviderFactory.cs (3)
IIndexStorageProvider(8-8)IIndexStorageProvider(15-15)IIndexStorageProvider(28-28)src/DotJEM.Json.Index2/Storage/IJsonIndexStorageManager.cs (2)
Delete(18-18)Delete(71-93)src/DotJEM.Json.Index2.Snapshots/Zip/ZipFileSnapshot.cs (1)
Delete(32-35)
src/DotJEM.Json.Index2/Leases/LeaseManager.cs (2)
src/DotJEM.Json.Index2/Leases/Lease.cs (3)
TOut(14-14)WithLock(15-15)TryRenew(12-12)src/DotJEM.Json.Index2/Util/Disposable.cs (2)
Dispose(9-17)Dispose(19-22)
🔇 Additional comments (6)
src/DotJEM.Json.Index2.Test/JsonIndexTest.cs (1)
86-86: Formatting change looks goodThis blank line addition maintains consistent spacing between test methods.
src/DotJEM.Json.Index2.Management/Writer/IJsonIndexWriter.cs (1)
154-155: Improved thread safety for commit operationsThe change from direct access to using
WithLockensures thread-safe commit operations by properly synchronizing access to the leased writer. This prevents potential race conditions when multiple threads attempt to commit simultaneously.src/DotJEM.Json.Index2/Leases/Lease.cs (1)
14-15: Good addition of thread-safe resource access methodsThese new methods provide a standardized way to perform operations on leased resources within a lock context, ensuring thread safety. The two method signatures accommodate both operations that return values and those that don't.
src/DotJEM.Json.Index2/Storage/IJsonIndexStorageManager.cs (2)
83-83: Good synchronization for deletion operationsAdding the WriterManager lock prevents new writers from being created during deletion, avoiding potential race conditions where writers might access files being deleted.
91-91: Proper lock release after deletionThe WriterManager is correctly unlocked after all deletion operations are complete, ensuring the system returns to normal operation.
src/DotJEM.Json.Index2.Management.Test/JsonIndexManagerTest.cs (1)
39-40: Verify that the explicitindex.Commit()does not undermine the test intentCommitting the index before
RunAsync()forces a writer to be created/closed outside the manager’s lifecycle, potentially invalidating the scenario “writer should not be disposed while the manager is running”.
Double-check that this early commit is really needed; otherwise remove it or move the call insideRunAsync()where the manager controls it.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/DotJEM.Json.Index2/IO/JsonIndexWriterManager.cs (2)
40-41:writersis still a non-thread-safe static listPrevious feedback already pointed out that concurrent
Addand enumeration can corrupt theList<T>instance (and enumeration at lines 111-113 may throw).
Protect the collection with a dedicated lock or swap it for a concurrent container.
42-49:Lock()/Unlock()should also holdwriterPadLockResetting the event without taking the same mutex used during writer creation leaves a race window:
- Thread-A enters
Writer, passesreset.Wait()(signalled), then is pre-empted.- Thread-B calls
Lock(), resets the event, and proceeds.- Thread-A resumes and creates a writer even though creation was supposed to be blocked.
Acquiring
writerPadLockinsideLock()/Unlock()(or movingreset.Wait()outside thewriterPadLocksection) would close this gap.
🧹 Nitpick comments (1)
src/DotJEM.Json.Index2/IO/JsonIndexWriterManager.cs (1)
63-67: Consider timeout / cancellation forreset.Wait()
reset.Wait()can block indefinitely if a caller forgets to callUnlock()(e.g., due to an exception).
Passing a reasonable timeout or aCancellationTokenwill make the system fail fast and easier to diagnose.Example:
reset.Wait(TimeSpan.FromSeconds(30), cancellationToken);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/DotJEM.Json.Index2/IO/JsonIndexWriterManager.cs(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/DotJEM.Json.Index2/IO/JsonIndexWriterManager.cs (5)
src/DotJEM.Json.Index2/Leases/LeaseManager.cs (9)
ILease(13-13)ILease(14-14)ILease(26-29)ILease(31-34)ILease(56-63)Lease(78-138)Lease(109-113)Dispose(129-137)Dispose(207-215)src/DotJEM.Json.Index2/Leases/Lease.cs (1)
ILease(18-18)src/DotJEM.Json.Index2/IO/IndexWriterSafeProxy.cs (2)
Dispose(35-40)Dispose(42-47)src/DotJEM.Json.Index2/IO/IIndexWriter.cs (2)
Dispose(14-14)Dispose(15-15)src/DotJEM.Json.Index2/Util/Disposable.cs (2)
Dispose(9-17)Dispose(19-22)
🔇 Additional comments (1)
src/DotJEM.Json.Index2/IO/JsonIndexWriterManager.cs (1)
76-82: 👍 Lease acquisition is now protected bywriterPadLockWrapping the lease creation in the same lock that guards
writerremoves a potential race between lease creation and writer replacement. Nice improvement.
1 participant
Summary by CodeRabbit