search

p-avital / token-cell-rs

An alternative to GhostCell
4 stars 1 forks source link

Possibly deprecate in favor of singleton-cell #2

Open jasoncarr0 opened 2 years ago

jasoncarr0 commented 2 years ago

Just letting you know, there's another remarkably similar crate (which I made, full disclosure) that solves the same use case. https://crates.io/crates/singleton-cell In addition, it handles the soundness issue of https://github.com/p-avital/token-cell-rs/issues/1 Although it is slightly larger (hopefully by not too much)

Would you like to consolidate (my hope with splitting out into a separate singleton-trait was to keep from having too many cell traits of the same sort)?

p-avital commented 2 years ago

Hi,

I'm aware of singleton-cell, and token-cell is indeed very similar. Hell, at first my tokens were also mandatory singletons (fixing the soundness hole), until I realized it didn't fit my use-case.

On singletons

The problem with mandatory singletons is that it doesn't address the case where multiple instances of an identical process must run concurrently. AFAIK, we can't statically prove at compile-time that 2 such concurrent instances will never try to access each other's data using their own token without enforcing constraints that would be incompatible with asynchronous runtimes. Since my "target audience" is asynchronous, multi-instance stuff, I can't quite enforce everything at comp-time, hence the soundness hole.

So my take is a bit more "trust but verify"-ish, I went for 1.0.0 WAY too early, which was dumb, as I know I'll change a lot of things when I get back to it, but here's my basic plan:

On consolidating

I'd love to. I admit I didn't approach you about it at first because my few recent experiences contributing even minor changes to crates I don't own have been a bit frustrating with delays (last few PRs took a couple weeks to get a response on a crate which prevented my team from updating other dependencies).

If you're willing to work together on this (maybe by adding the approach I mentioned to your crate), I'd be glad to.

Alternatively, if you want to keep your crate soundness-hole-free, I could change my crate's communication a bit, pointing people who don't need multi-instanciable 'static tokens towards your crate or ghost-cell. I do find this solution inferior to proper consolidation though.

p-avital commented 2 years ago

Update: I found some time and motivation. Feel free to look at the latest commit to check out what I had in mind for the future of token-cell :)

TLDR: token-cell now has GhostToken<'brand>, a singleton_token! generator, a runtime_token! generator (and exports a RuntimeToken type), and an unsafe_token! generator.
The default token! generator generates runtime tokens when the debug feature is enabled (which it is by default) and unsafe tokens otherwise.

jasoncarr0 commented 2 years ago

Got it.

I've looked over your recent changes, I think there's some good decisions although they've diverged from what I have so I'm not sure they'd be compatible. With the fallibility it looks a bit more like https://crates.io/crates/tokenlock although I find that crate very complex. By its nature my crate is more of 0-cost transparent cells only, and true singletons, so it wouldn't be compatible. Technically one solution that gives up the runtime checking could be to unsafe impl Singleton for an unsafe token type and get the SCell interface over it for free. If you're using this in an application, that's a very fast and simple approach.

On the topic of soundness: In the case that you have a soundness hole (as you do), you should always mark the functions which can cause it as unsafe. It's probably not going to be too clear how to avoid the soundness, but it is the way to indicate opting out from safety, and adds at most 8 characters to the code that has to call it.

It's probably sufficient to make creation of the unsafe token be an unsafe operation. The safety condition doesn't make too much sense and so is still a bit of foot-gunny, but it makes it possible to keep the access operations safe and forces an unsafe block to be used somewhere. I wouldn't release any known unsound code without that minimum.