TimelyDataflow / abomonation

A mortifying serialization library for Rust
MIT License
317 stars 30 forks source link

Shouldn't decode return an &mut T rather than a &T? #19

Open HadrienG2 opened 5 years ago

HadrienG2 commented 5 years ago

As far as I can see, there is no reason why users shouldn't be allowed to modify the deserialized object in place if they so desire.

frankmcsherry commented 5 years ago

For some types this is ok, but for other types like Vec<T> the ability to invoke &mut method could prompt e.g. de-allocation of memory that the allocator is not expecting (in this case, halfway through some bytes array). You would also gain access to mem::swap and other methods that replace the serialized data with owned data, and would complicate the state of the world once the &mut T is dropped (a bunch of owned data behind that should be cleaned up).

Does that make sense / were you thinking of a different reason?

HadrienG2 commented 5 years ago

Oh, I didn't realize that abomonation replaces the memory allocations of RAII objects with inline storage in its buffer. But in hindsight, it makes total sense, given how the exhume API looks.

A first question that comes to my mind is how you manage to get the pointer alignment right, but I guess this is already the topic of #8.

Also, this means that one must be extremely careful about implementing Abomonation for types that use internal mutability. It would probably be a good idea to mention this in the Safety section of the Abomonation trait documentation.

frankmcsherry commented 5 years ago

Yup. Some of these things have quieted a bit as it became (years back) clear that Rust didn't have a stable story around unsafe, and I didn't want to make specific claims until they sorted that out more. Mostly, I didn't want to give the false impression that watching out for a few footguns would mean safety ensues. Instead, the front-page warning of "do not use this on data you care about" is meant to cover that. Also, text like

Be warned that implementing Abomonable for types can be a giant disaster and is entirely discouraged.

though it looks like I have the wrong name there.

I'll add some text to the trait documentation after "do not call these methods" and before "maybe don't use the trait at all" warnings. :)

frankmcsherry commented 5 years ago

Btw, are you using this for something, or considering it? The crate exists mostly for a small-ish set of performance sensitive people working with big data, and it has other issues that limit broad applicability (e.g. no HashMap implementation, some other key owned library types). It's something I would love to make a bit more serious, but am blocked on the Rust unsafe team for a while, I think.

HadrienG2 commented 5 years ago

I'm currently designing a log backend for real-time applications, and investigating the use of abomonation as a maximally-lightweight serialization mechanism for transferring log records from the real-time threads to the non-real-time logger thread. It seems that it could fit this sort of need better than a general-purpose mechanism like serde, and as a bonus it also gives me the "how large will serialized data be" info that I need for my evil lock-free programming tricks.

But obviously, before actually using it, I had to go beyond your "this library will eat your data and summon nasal demons" blanket statements and get a reasonably solid understanding of what abomonation's actual safety preconditions are. And this is how I ended up studying your API and spamming your bug tracker in the process ;)

By the way, you may be interested in the unsafe-code-guidelines repo if you don't know about it already. There were a lot of interesting discussions in there recently, and compiler people are available there to answer questions about how rustc currently behaves and whether they see a reasonable chance of it changing in the future.

HadrienG2 commented 4 years ago

I think you should take a look at this RustConf talk by @oli-obk. The notion of ConstRefSafe that they define for the purpose of allowing heap allocations in const-eval seems suspiciously similar to the type contract that you need for replacing heap allocations with inline allocations in abomonation...