https://bugs.openjdk.org/browse/JDK-8316337 addresses a concurrency issue with DirectByteBuffer.Deallocator utilising the immutability characteristics of Records types to provide thread safety. This is not without potential side effects, as noted below:
As per comment inJDK-8316337, this achieves a resolution of a perceived concurrent access issue in the run method's transitioning of a Dealloactor's internal state (as per its member variables defined in its definition). However, this may introduce a side effect, and possible potential problem. The internal Deallocator state (address) is "referencing" a block of memory being used. The Dealloactor run method frees this memory. In the original the state of that memory was tested to see if it was still allocated, i.e. address != 0, and if so then freed the memory. Then after the free operation the address set to 0 to indicate the memory is no longer allocated.
With the current change that test is no longer performed, as the state is immutable, and the memory will always be freed in the invocation of run. Thus, this fix assumes that UNSAFE.freeMemory is idempotent. If UNSAFE.freeMemory maps to a native memory free call, then that assumption may not be valid, and behaviour can be unspecified, which may lead to "peculiar behaviour".
Additionally, one can conceive a (,somewhat contrived ) scenario of concurrent invocations of a Deallocator::run, first run frees the referenced memory, the memory is returned to native allocator and re-allocated before the second concurrent run is invoked. Then the second concurrent Deallocator::run is invoked and that frees the already freed but now reallocated memory, with unforeseen side effects of that memory being unintentionally freed.
So the defined state of the Deallocator is not just the member variables, but the state of the underlying native memory implicitly referenced by address, and if there are multiple (concurrent or sequential) calls on Deallocator::run, resulting in multiple calls on os::free, it results in unspecified behaviour.
Now, there are “shouts from the gallery", that this is academic, long standing issue, or that multiple concurrent calls can’t be made, as per Cleaner semantics. Those claims not withstanding, the refactoring and restructuring change has been made to address thread safety, and the challenge here is that it has a potential flaw. Thus, in accepting there was concurrency issue in DirectByteBuffer.Deallocator, then thread safety should be asserted regardless of what is currently concurrently possible.
of course this can be addressed as WNF or not an issue, but that would fail to address the correctness of the current change.
As per comment in
With the current change that test is no longer performed, as the state is immutable, and the memory will always be freed in the invocation of run. Thus, this fix assumes that UNSAFE.freeMemory is idempotent. If UNSAFE.freeMemory maps to a native memory free call, then that assumption may not be valid, and behaviour can be unspecified, which may lead to "peculiar behaviour".
Additionally, one can conceive a (,somewhat contrived ) scenario of concurrent invocations of a Deallocator::run, first run frees the referenced memory, the memory is returned to native allocator and re-allocated before the second concurrent run is invoked. Then the second concurrent Deallocator::run is invoked and that frees the already freed but now reallocated memory, with unforeseen side effects of that memory being unintentionally freed.
So the defined state of the Deallocator is not just the member variables, but the state of the underlying native memory implicitly referenced by address, and if there are multiple (concurrent or sequential) calls on Deallocator::run, resulting in multiple calls on os::free, it results in unspecified behaviour.
Now, there are “shouts from the gallery", that this is academic, long standing issue, or that multiple concurrent calls can’t be made, as per Cleaner semantics. Those claims not withstanding, the refactoring and restructuring change has been made to address thread safety, and the challenge here is that it has a potential flaw. Thus, in accepting there was concurrency issue in DirectByteBuffer.Deallocator, then thread safety should be asserted regardless of what is currently concurrently possible.
of course this can be addressed as WNF or not an issue, but that would fail to address the correctness of the current change.
- relates to
-
JDK-8316337 (bf) Concurrency issue in DirectByteBuffer.Deallocator
-
- Closed
-
- links to
-
Review openjdk/jdk/17647