Java cleaner synchronization [skip ci] #7474
Conversation
abellina
changed the title
|
I am running some tests on this but posting as draft for now. |
|
I am not seeing unit test failures as a result of this change in cudf or the Spark plugin, |
| @@ -114,7 +114,7 @@ public String toString() { | |||
| } | |||
|
|
|||
| @Override | |||
| public void close() { | |||
| public synchronized void close() { | |||
There was a problem hiding this comment.
Why do we need to synchronize closing a stream, but none of the other classes?
There was a problem hiding this comment.
Most of the other classes synchronized on close, except Stream and Event. As of this PR, these don't synchronize:
HostColumnVectorCore(as far as as I can tell itscloseis overwritten by subclasses)- nvcomp:
BatchedMetadataandMetadata. Neither of these are called in a multi-threaded context.
I think we can synchronize close for the nvcomp ones, that seems like it could be a miss later. If you think this is unnecessary let me know and I can revert parts of this PR.
There was a problem hiding this comment.
HostColumnVectorCore(as far as as I can tell itscloseis overwritten by subclasses)
Child columns are instances of this class. So we should probably add synchronized to the close in HostColumnVectorCore.
There was a problem hiding this comment.
@revans2 did you want me to make a change around this?
There was a problem hiding this comment.
ok done. I need to re-run my tests after upmerging. FYI.
|
@abellina Please edit the PR description (which becomes part of the commit message) to drop the mention of a draft and I think this is good to go. |
Done! |
|
@gpucibot merge |
Add synchronization in `cleanImpl` and `close` in various places where race conditions could exist, and also within the `MemoryCleaner` to address some concurrent modification issues we've seen in tests while shutting down (i.e. invoking the cleaner) (i.e. NVIDIA/spark-rapids#1797) Authors: - Alessandro Bellina (@abellina) Approvers: - Robert (Bobby) Evans (@revans2) - Jason Lowe (@jlowe) URL: rapidsai#7474
Add synchronization in
cleanImplandclosein various places where race conditions could exist, and also within theMemoryCleanerto address some concurrent modification issues we've seen in tests while shutting down (i.e. invoking the cleaner) (i.e. NVIDIA/spark-rapids#1797)