CBL-4435:Replicator.close() stops status updates #194
Conversation
| // I have chosen #3. My rational is that, if you create and start a replicator and then null out | ||
| // all references to it, this is exactly what will happen: the replicator will be forgotten and | ||
| // eventually, GCed, told to stop, and freed. This method does precisely that: best effort to | ||
| // release the resources and then forget about them. |
There was a problem hiding this comment.
My impression that close() method has synchronize behavior, and the replicator will be stopped and its resource will be released. So maybe #1 with synchronous behavior to ensure that the replicator is stopped (similar to database.close())?
Also, I think the Java and .NET could be aligned on the behavior and implementation. CC: @borrrden.
There was a problem hiding this comment.
As I note in the comments, I don't like the idea that the close method will leave the Replicator in a half-dead state until the Replicator finishes. I think that would be a big mistake.
To reiterate the thought experiment: Consider what happens if client code creates a replicator, and then deletes all references to it. That is important, because, I maintain, the exact purpose of the close() method is to do, under user (client code) control, exactly what happens if the GC gets the object.
This implementation does exactly that. It is exactly as if the replicator were GCed: It stops the replicator, best effort and forgets the companion object.
There was a problem hiding this comment.
As I note in the comments, I don't like the idea that the close method will leave the Replicator in a half-dead state until the Replicator finishes. I think that would be a big mistake.
I'm not sure what it means by leaving the replicator in half-dead state if the close method waits until the replicator is stopped like database.close() does?
There was a problem hiding this comment.
I see. Sure, I could do that: no zombie state. It might take minutes, though. I have three arguments against that idea:
- I think Java devs would find it quite surprising if a
close()method took minutes to run. Not a blocker, but surprising. - There is no way that we could wait for the close, in the finalizer. That would be catastrophic. If close is going to do what GC does (which I claim it should), it should not wait.
- the reason you would wait is in case that the replicator did not close cleanly. But what would client code do with that information? I submit that the best effort attempt will usually work, and that when it doesn't work, there's nothing that the user can do, anyway.
The only argument I can see for waiting is that client code wants to do something that they can only do, once the replicator is definitely stopped. There is a perfectly straightforward way to do that, right now without the close method: attach a listener and wait for the stopped state.
| super.close(); | ||
|
|
||
| if (newStatus == null) { return; } | ||
| postChange(true, new ReplicatorChange((Replicator) this, newStatus), listeners); |
There was a problem hiding this comment.
I guessed super.close() will just release the LiteCore's replicator. Could this cause a crash if replicator is running?
I'm not sure about the behavior here when notifying the STOPPED, but the replicator may not be stopped yet? Will it be possible to get notified with BUSY or something else after STOPPED is notified.
There was a problem hiding this comment.
It first calls c4repl_stop then c4repl_free.
I have two reasons to believe that this won't cause crashes:
- I believe that LiteCore hold references to the replicator. I believe I just decrement the ref count. I should be confirm this.
- All replicators are currently released in exactly the way that this close method releases them. It's been this way since 3.0. If this way of releasing them might cause crashes, I think we'd have seen them by now.
There was a problem hiding this comment.
The replicator is, absolutely, stopped, as far as Java is concerned. It no longer holds any references to the LiteCore replicator object, and the LiteCore object has no valid references to the Java objects. If the LiteCore replicator attempts to report any status change (or anything like that) to the Java code, the attempt will simply be ignored.
|
Added JavaDoc comments, as recommended by @pasin and also an |
|
G. Blake Meike: Jens Alfke: |
An extensive comment describes the choice I mede for implementation.
tl; dr: I believe the intention of the close() method is to do exactly what would happen if the GC got the object, but to do it right now. That is exactly what this implementation does. It tells LiteCore to close the replicator and then completely forgets about it. No zombie objects, no long delays, no unexpected exceptions.