Ensure resource ids do not ever repeat/got re-used #23
Conversation
dr-orlovsky
added
bug
src/resource.rs
Outdated
| pub trait ResourceId: Copy + Eq + Ord + Hash + Send + Debug + Display {} | ||
| /// The resource identifier must be globally unique and non-reusable object. Because of this, | ||
| /// things like [`RawFd`] and socket addresses can't operate like resource identifiers. | ||
| pub type ResourceId = u64; |
There was a problem hiding this comment.
Might be a good idea to make this a pub struct ResourceId(u64) to prevent application code from constructing a non-existent resource-id by mistake?
There was a problem hiding this comment.
Yes, that was the plan. But first I'd like to make sure that everything in heartwood works with this approach - and it isn't. But I can't get your review there since I fail to submit a patch - we just discussed that in chat
|
This looks good, just that one comment on type safety. |
src/resource.rs
Outdated
| impl ResourceId { | ||
| pub(crate) const ZERO: ResourceId = ResourceId(0); | ||
|
|
||
| pub(crate) fn inc(&mut self) { self.0 += 1 } |
There was a problem hiding this comment.
Not really a fan of exposing a method like this, which is why I usually try to encapsulate the whole incrementation logic inside the type/module using a static variable; but if you prefer to do it this way then 🤷
There was a problem hiding this comment.
static variable doesn't solve this. If we do not have this I would have to make ResourceId(pub(crate) u64), which I think is worse - or provide instead of inc pub(crate) constructor
There was a problem hiding this comment.
Ok, I've got what you mean: put static variable inside ResourceId constructor.
That's not good, since now resource id is unique on a per-reactor basis. The change will make it globally unique for all io-reactors. At the same time, each reactor assigns zero to its waker, and this will not work with globally-unique ids.
There was a problem hiding this comment.
Also not sure whether introducing atomic type worth for just APIs means: atomics (while lock-free) and not wait-free, and will "entangle" different reactors waiting for each other
There was a problem hiding this comment.
Are you saying there is a valid use-case for running more than one reactor per process? I can't really think of any, but another solution would be to put the atomic var inside a thread_local! block.
There was a problem hiding this comment.
Yes, there is: I had two reactors in the nsh program when I was doing tunnelling and general connectivity. I do not remember the case well, but I was definitely needing two of them.
I do not understand why doing thread_local! and atomic variables, which are runtime is better than compile-time visibility limitation...
There was a problem hiding this comment.
Ah yes, because you had a sub-process maybe.
The issue I have with the current implementation is this:
let mut id1 = reactor.register();
id1.incr(); // Now this id doesn't represent an existing resource
let mut id2 = reactor.register();
// Now id1 and id2 point to the same resource
Ideally, an external module should not be allowed to generate invalid IDs, or create collisions. This would be solved if the resource module would generate immutable ResourceIDs.
One idea would be to have a resource::ResourceIdGenerator type and instantiate one of them per poller, and keep id_top in there.
Then the poller is guaranteed to get each ID only once, and no module can mutate existing IDs.
There was a problem hiding this comment.
Ok, I have re-done the mechanism of generating the resource id according to your suggestion. Please let me know what you think
dr-orlovsky
force-pushed
the
resourceid
branch
from
efde1d1
to
e337475
Compare
I was able to find a way not even to use atomic counter. Seems like this simplifies APIs and had to be done from the very beginning in such a way.