You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "adoroszlai (via GitHub)" <gi...@apache.org> on 2023/05/18 09:06:30 UTC
[GitHub] [ozone] adoroszlai opened a new pull request, #4731: HDDS-8476. ManagedColumnFamilyOptions is not closed properly
adoroszlai opened a new pull request, #4731:
URL: https://github.com/apache/ozone/pull/4731
## What changes were proposed in this pull request?
`DatanodeDBProfile` may leak `ManagedColumnFamilyOptions` and its related objects:
```
[Finalizer] WARN managed.ManagedRocksObjectUtils (ManagedRocksObjectUtils.java:assertClosed(54)) - ManagedLRUCache is not closed properly
[Finalizer] WARN managed.ManagedRocksObjectUtils (ManagedRocksObjectUtils.java:assertClosed(54)) - ManagedBloomFilter is not closed properly
[Finalizer] WARN managed.ManagedRocksObjectUtils (ManagedRocksObjectUtils.java:assertClosed(54)) - ManagedColumnFamilyOptions is not closed properly
```
`AtomicReference#updateAndGet` requires the update function to be free of side-effects, since it is applied in a loop and may be executed multiple times until `compareAndSet` succeeds. However, `op -> op != null ? op : createColumnFamilyOptions(config))` has side-effect: the creation of a `ManagedColumnFamilyOptions`. The objects created in unsuccessful attempts are leaked.
`op != null ? op : ...` indicates we only want to replace `null` value, any non-`null` value already set is OK. So `updateAndGet` can be replaced with a single `compareAndSet(null, ...)`. If `non-null` value is found during set, the new instance needs to be closed. Once a non-`null` value is set, it is never replaced, so we can return it early to avoid unnecessary object creation.
https://issues.apache.org/jira/browse/HDDS-8476
## How was this patch tested?
Added debug log in `DatanodeDBProfile` to show `ManagedColumnFamilyOptions` instances being created and returned. Verified that the test does not fail due to leaks even if two instances of `ManagedColumnFamilyOptions` were created in `getColumnFamilyOptions` -- one was used later for each call, the other was closed properly right after creation.
CI:
https://github.com/adoroszlai/hadoop-ozone/actions/runs/5011701073
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #4731: HDDS-8476. ManagedColumnFamilyOptions is not closed properly
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #4731:
URL: https://github.com/apache/ozone/pull/4731#issuecomment-1553559128
Thanks @szetszwo for the suggestion, it's definitely more elegant.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] szetszwo merged pull request #4731: HDDS-8476. ManagedColumnFamilyOptions is not closed properly in DatanodeDBProfile
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo merged PR #4731:
URL: https://github.com/apache/ozone/pull/4731
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] szetszwo commented on pull request #4731: HDDS-8476. ManagedColumnFamilyOptions is not closed properly
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #4731:
URL: https://github.com/apache/ozone/pull/4731#issuecomment-1553289523
@adoroszlai , a standard solution is to use [Double-checked locking](https://en.wikipedia.org/wiki/Double-checked_locking) or a `MemoizedSupplier` as shown below.
```java
@@ -25,8 +25,10 @@
import org.apache.hadoop.hdds.utils.db.managed.ManagedColumnFamilyOptions;
import org.apache.hadoop.hdds.utils.db.managed.ManagedDBOptions;
import org.apache.hadoop.hdds.utils.db.managed.ManagedLRUCache;
+import org.apache.ratis.util.MemoizedSupplier;
import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Supplier;
import static org.apache.hadoop.ozone.OzoneConfigKeys.HDDS_DATANODE_METADATA_ROCKSDB_CACHE_SIZE;
import static org.apache.hadoop.ozone.OzoneConfigKeys.HDDS_DATANODE_METADATA_ROCKSDB_CACHE_SIZE_DEFAULT;
@@ -107,8 +109,8 @@ public ManagedColumnFamilyOptions getColumnFamilyOptions(
* Base profile for datanode storage disks.
*/
private static final class StorageBasedProfile {
- private final AtomicReference<ManagedColumnFamilyOptions> cfOptions =
- new AtomicReference<>();
+ private final AtomicReference<Supplier<ManagedColumnFamilyOptions>>
+ cfOptions = new AtomicReference<>();
private final DBProfile baseProfile;
private StorageBasedProfile(DBProfile profile) {
@@ -121,9 +123,10 @@ private ManagedDBOptions getDBOptions() {
private ManagedColumnFamilyOptions getColumnFamilyOptions(
ConfigurationSource config) {
- cfOptions.updateAndGet(op -> op != null ? op :
- createColumnFamilyOptions(config));
- return cfOptions.get();
+ final Supplier<ManagedColumnFamilyOptions> options
+ = MemoizedSupplier.valueOf(() -> createColumnFamilyOptions(config));
+ cfOptions.compareAndSet(null, options);
+ return cfOptions.get().get();
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org