Skip to content

Commit 16895b3

Browse files
mulimoenmulimoen
committed
Remove Registry
1 parent 9445078 commit 16895b3

File tree

4 files changed

+124
-106
lines changed

4 files changed

+124
-106
lines changed

‎src/handle.rs

Lines changed: 16 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,3 @@
1-
use std::collections::HashMap;
2-
use std::sync::Arc;
3-
4-
use lazy_static::lazy_static;
5-
use parking_lot::{Mutex, RwLock};
6-
71
use hdf5_sys::h5i::{H5I_type_t, H5Idec_ref, H5Iget_type, H5Iinc_ref, H5Iis_valid};
82

93
use crate::internal_prelude::*;
@@ -20,6 +14,10 @@ pub fn get_id_type(id: hid_t) -> H5I_type_t {
2014
})
2115
}
2216

17+
pub(crate) fn refcount(id: hid_t) -> Result<hsize_t> {
18+
h5call!(hdf5_sys::h5i::H5Iget_ref(id)).map(|x| x as _)
19+
}
20+
2321
pub fn is_valid_id(id: hid_t) -> bool {
2422
h5lock!({
2523
let tp = get_id_type(id);
@@ -31,60 +29,29 @@ pub fn is_valid_user_id(id: hid_t) -> bool {
3129
h5lock!({ H5Iis_valid(id) == 1 })
3230
}
3331

34-
struct Registry {
35-
registry: Mutex<HashMap<hid_t, Arc<RwLock<hid_t>>>>,
36-
}
37-
38-
impl Default for Registry {
39-
fn default() -> Self {
40-
Self::new()
41-
}
42-
}
43-
44-
impl Registry {
45-
pub fn new() -> Self {
46-
Self { registry: Mutex::new(HashMap::new()) }
47-
}
48-
49-
pub fn new_handle(&self, id: hid_t) -> Arc<RwLock<hid_t>> {
50-
let mut registry = self.registry.lock();
51-
let handle = registry.entry(id).or_insert_with(|| Arc::new(RwLock::new(id)));
52-
if *handle.read() != id {
53-
// an id may be left dangling by previous invalidation of a linked handle
54-
*handle = Arc::new(RwLock::new(id));
55-
}
56-
handle.clone()
57-
}
58-
}
59-
32+
/// A handle to a `hdf5` object
6033
pub struct Handle {
61-
id: Arc<RwLock<hid_t>>,
34+
id: hid_t,
6235
}
6336

6437
impl Handle {
38+
/// Take ownership of the object id
6539
pub fn try_new(id: hid_t) -> Result<Self> {
66-
lazy_static! {
67-
static ref REGISTRY: Registry = Registry::new();
68-
}
6940
h5lock!({
7041
if is_valid_user_id(id) {
71-
Ok(Self { id: REGISTRY.new_handle(id) })
42+
Ok(Self { id })
7243
} else {
7344
Err(From::from(format!("Invalid handle id: {}", id)))
7445
}
7546
})
7647
}
7748

7849
pub fn invalid() -> Self {
79-
Self { id: Arc::new(RwLock::new(H5I_INVALID_HID)) }
50+
Self { id: H5I_INVALID_HID }
8051
}
8152

8253
pub fn id(&self) -> hid_t {
83-
*self.id.read()
84-
}
85-
86-
pub fn invalidate(&self) {
87-
*self.id.write() = H5I_INVALID_HID;
54+
self.id
8855
}
8956

9057
pub fn incref(&self) {
@@ -93,16 +60,14 @@ impl Handle {
9360
}
9461
}
9562

96-
pub fn decref(&self) {
63+
/// An object should not be decreffed unless it has an
64+
/// associated incref
65+
pub unsafe fn decref(&self) {
9766
h5lock!({
9867
if self.is_valid_id() {
9968
H5Idec_ref(self.id());
10069
}
101-
// must invalidate all linked IDs because the library reuses them internally
102-
if !self.is_valid_user_id() && !self.is_valid_id() {
103-
self.invalidate();
104-
}
105-
});
70+
})
10671
}
10772

10873
/// Returns `true` if the object has a valid unlocked identifier (`false` for pre-defined
@@ -115,10 +80,8 @@ impl Handle {
11580
is_valid_id(self.id())
11681
}
11782

118-
pub fn decref_full(&self) {
119-
while self.is_valid_user_id() {
120-
self.decref();
121-
}
83+
pub(crate) fn refcount(&self) -> u32 {
84+
refcount(self.id).unwrap_or(0) as u32
12285
}
12386
}
12487

‎src/hl/file.rs

Lines changed: 79 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,7 @@ use std::path::Path;
55
use hdf5_sys::h5f::{
66
H5Fclose, H5Fcreate, H5Fflush, H5Fget_access_plist, H5Fget_create_plist, H5Fget_filesize,
77
H5Fget_freespace, H5Fget_intent, H5Fget_obj_count, H5Fget_obj_ids, H5Fopen, H5F_ACC_DEFAULT,
8-
H5F_ACC_EXCL, H5F_ACC_RDONLY, H5F_ACC_RDWR, H5F_ACC_TRUNC, H5F_OBJ_ALL, H5F_OBJ_FILE,
9-
H5F_SCOPE_LOCAL,
8+
H5F_ACC_EXCL, H5F_ACC_RDONLY, H5F_ACC_RDWR, H5F_ACC_TRUNC, H5F_SCOPE_LOCAL,
109
};
1110

1211
use crate::hl::plist::{
@@ -133,6 +132,7 @@ impl File {
133132
}
134133

135134
/// Returns objects IDs of the contained objects. NOTE: these are borrowed references.
135+
#[allow(unused)]
136136
fn get_obj_ids(&self, types: c_uint) -> Vec<hid_t> {
137137
h5lock!({
138138
let count = h5call!(H5Fget_obj_count(self.id(), types)).unwrap_or(0) as size_t;
@@ -151,26 +151,11 @@ impl File {
151151
}
152152

153153
/// Closes the file and invalidates all open handles for contained objects.
154-
pub fn close(self) {
155-
h5lock!({
156-
let file_ids = self.get_obj_ids(H5F_OBJ_FILE);
157-
let object_ids = self.get_obj_ids(H5F_OBJ_ALL & !H5F_OBJ_FILE);
158-
for file_id in &file_ids {
159-
if let Ok(handle) = Handle::try_new(*file_id) {
160-
handle.decref_full();
161-
}
162-
}
163-
for object_id in &object_ids {
164-
if let Ok(handle) = Handle::try_new(*object_id) {
165-
handle.decref_full();
166-
}
167-
}
168-
H5Fclose(self.id());
169-
while self.is_valid() {
170-
self.0.decref();
171-
}
172-
self.0.decref();
173-
});
154+
pub fn close(self) -> Result<()> {
155+
let id = self.id();
156+
// Ensure we only decref once
157+
std::mem::forget(self.0);
158+
h5call!(H5Fclose(id)).map(|_| ())
174159
}
175160

176161
/// Returns a copy of the file access property list.
@@ -500,29 +485,86 @@ pub mod tests {
500485
}
501486

502487
#[test]
503-
pub fn test_close_automatic() {
504-
// File going out of scope should just close its own handle
488+
fn test_strong_close() {
489+
use crate::hl::plist::file_access::FileCloseDegree;
505490
with_tmp_path(|path| {
506-
let file = File::create(&path).unwrap();
491+
let file = File::with_options()
492+
.with_fapl(|fapl| fapl.fclose_degree(FileCloseDegree::Strong))
493+
.create(&path)
494+
.unwrap();
495+
assert_eq!(file.refcount(), 1);
496+
let fileid = file.id();
497+
507498
let group = file.create_group("foo").unwrap();
499+
assert_eq!(file.refcount(), 1);
500+
assert_eq!(group.refcount(), 1);
501+
508502
let file_copy = group.file().unwrap();
503+
assert_eq!(group.refcount(), 1);
504+
assert_eq!(file.refcount(), 2);
505+
assert_eq!(file_copy.refcount(), 2);
506+
509507
drop(file);
510-
assert!(group.is_valid());
511-
assert!(file_copy.is_valid());
508+
assert_eq!(crate::handle::refcount(fileid).unwrap(), 1);
509+
assert_eq!(group.refcount(), 1);
510+
assert_eq!(file_copy.refcount(), 1);
511+
512+
h5lock!({
513+
// Lock to ensure fileid does not get overwritten
514+
let groupid = group.id();
515+
drop(file_copy);
516+
assert!(crate::handle::refcount(fileid).is_err());
517+
assert!(crate::handle::refcount(groupid).is_err());
518+
assert!(!group.is_valid());
519+
});
512520
});
513521
}
514522

515523
#[test]
516-
pub fn test_close_manual() {
517-
// File::close() should close handles of all related objects
524+
fn test_weak_close() {
525+
use crate::hl::plist::file_access::FileCloseDegree;
526+
with_tmp_path(|path| {
527+
let file = File::with_options()
528+
.with_fapl(|fapl| fapl.fclose_degree(FileCloseDegree::Weak))
529+
.create(&path)
530+
.unwrap();
531+
assert_eq!(file.refcount(), 1);
532+
let fileid = file.id();
533+
534+
let group = file.create_group("foo").unwrap();
535+
assert_eq!(file.refcount(), 1);
536+
assert_eq!(group.refcount(), 1);
537+
538+
let file_copy = group.file().unwrap();
539+
assert_eq!(group.refcount(), 1);
540+
assert_eq!(file.refcount(), 2);
541+
assert_eq!(file_copy.refcount(), 2);
542+
543+
drop(file);
544+
assert_eq!(crate::handle::refcount(fileid).unwrap(), 1);
545+
assert_eq!(group.refcount(), 1);
546+
assert_eq!(file_copy.refcount(), 1);
547+
548+
h5lock!({
549+
// Lock to ensure fileid does not get overwritten
550+
drop(file_copy);
551+
assert!(crate::handle::refcount(fileid).is_err());
552+
});
553+
assert_eq!(group.refcount(), 1);
554+
});
555+
}
556+
557+
#[test]
558+
pub fn test_close_automatic() {
559+
// File going out of scope should just close its own handle
518560
with_tmp_path(|path| {
519561
let file = File::create(&path).unwrap();
520562
let group = file.create_group("foo").unwrap();
521563
let file_copy = group.file().unwrap();
522-
file.close();
523-
assert!(!group.is_valid());
524-
assert!(!file_copy.is_valid());
525-
})
564+
drop(file);
565+
assert!(group.is_valid());
566+
assert!(file_copy.is_valid());
567+
});
526568
}
527569

528570
#[test]
@@ -532,7 +574,7 @@ pub mod tests {
532574
FileBuilder::new().with_fapl(|p| p.core_filebacked(false)).create(&path).unwrap();
533575
file.create_group("x").unwrap();
534576
assert!(file.is_valid());
535-
file.close();
577+
file.close().unwrap();
536578
assert!(fs::metadata(&path).is_err());
537579
assert_err!(
538580
FileBuilder::new().with_fapl(|p| p.core()).open(&path),
@@ -548,7 +590,7 @@ pub mod tests {
548590
FileBuilder::new().with_fapl(|p| p.core_filebacked(true)).create(&path).unwrap();
549591
assert!(file.is_valid());
550592
file.create_group("bar").unwrap();
551-
file.close();
593+
file.close().unwrap();
552594
assert!(fs::metadata(&path).is_ok());
553595
File::open(&path).unwrap().group("bar").unwrap();
554596
})
@@ -594,8 +636,8 @@ pub mod tests {
594636
let path = dir.join("qwe.h5");
595637
let file = File::create(&path).unwrap();
596638
assert_eq!(format!("{:?}", file), "<HDF5 file: \"qwe.h5\" (read/write)>");
597-
let root = file.file().unwrap();
598-
file.close();
639+
file.close().unwrap();
640+
let root = File::from_handle(Handle::invalid());
599641
assert_eq!(format!("{:?}", root), "<HDF5 file: invalid id>");
600642
let file = File::open(&path).unwrap();
601643
assert_eq!(format!("{:?}", file), "<HDF5 file: \"qwe.h5\" (read-only)>");

‎src/hl/group.rs

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -252,7 +252,12 @@ pub mod tests {
252252

253253
#[test]
254254
pub fn test_debug() {
255-
with_tmp_file(|file| {
255+
use crate::hl::plist::file_access::FileCloseDegree;
256+
with_tmp_path(|path| {
257+
let file = File::with_options()
258+
.with_fapl(|fapl| fapl.fclose_degree(FileCloseDegree::Strong))
259+
.create(&path)
260+
.unwrap();
256261
file.create_group("a/b/c").unwrap();
257262
file.create_group("/a/d").unwrap();
258263
let a = file.group("a").unwrap();
@@ -261,8 +266,10 @@ pub mod tests {
261266
assert_eq!(format!("{:?}", a), "<HDF5 group: \"/a\" (2 members)>");
262267
assert_eq!(format!("{:?}", ab), "<HDF5 group: \"/a/b\" (1 member)>");
263268
assert_eq!(format!("{:?}", abc), "<HDF5 group: \"/a/b/c\" (empty)>");
264-
file.close();
265-
assert_eq!(format!("{:?}", a), "<HDF5 group: invalid id>");
269+
h5lock!({
270+
file.close().unwrap();
271+
assert_eq!(format!("{:?}", a), "<HDF5 group: invalid id>");
272+
})
266273
})
267274
}
268275

‎src/hl/object.rs

Lines changed: 19 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,5 @@
11
use std::fmt::{self, Debug};
22

3-
use hdf5_sys::h5i::H5Iget_ref;
4-
53
use crate::internal_prelude::*;
64

75
/// Any HDF5 object that can be referenced through an identifier.
@@ -37,11 +35,7 @@ impl Object {
3735

3836
/// Returns reference count if the handle is valid and 0 otherwise.
3937
pub fn refcount(&self) -> u32 {
40-
if self.is_valid() {
41-
h5call!(H5Iget_ref(self.id())).unwrap_or(0) as _
42-
} else {
43-
0
44-
}
38+
self.handle().refcount()
4539
}
4640

4741
/// Returns `true` if the object has a valid unlocked identifier (`false` for pre-defined
@@ -105,7 +99,7 @@ pub mod tests {
10599
}
106100

107101
fn decref(&self) {
108-
self.0.decref()
102+
unsafe { self.0.decref() }
109103
}
110104
}
111105

@@ -148,18 +142,30 @@ pub mod tests {
148142
assert!(is_valid_id(obj.id()));
149143
assert!(is_valid_user_id(obj.id()));
150144
assert_eq!(obj.refcount(), 1);
151-
let mut obj2 = TestObject::from_id(obj.id()).unwrap();
145+
146+
let obj2 = TestObject::from_id(obj.id()).unwrap();
152147
obj2.incref();
153148
assert_eq!(obj.refcount(), 2);
154149
assert_eq!(obj2.refcount(), 2);
150+
155151
drop(obj2);
156152
assert!(obj.is_valid());
157153
assert_eq!(obj.refcount(), 1);
158-
obj2 = TestObject::from_id(obj.id()).unwrap();
154+
155+
// obj is already owned, we must ensure we do not call drop on this without
156+
// an incref
157+
let mut obj2 = std::mem::ManuallyDrop::new(TestObject::from_id(obj.id()).unwrap());
158+
assert_eq!(obj.refcount(), 1);
159+
159160
obj2.incref();
161+
// We can now take, as we have exactly two handles
162+
let obj2 = unsafe { std::mem::ManuallyDrop::take(&mut obj2) };
163+
160164
obj.decref();
161-
obj.decref();
162-
assert_eq!(obj.id(), H5I_INVALID_HID);
163-
assert_eq!(obj2.id(), H5I_INVALID_HID);
165+
h5lock!({
166+
obj.decref();
167+
assert!(!obj.is_valid());
168+
assert!(!obj2.is_valid());
169+
});
164170
}
165171
}

0 commit comments

Comments
 (0)