add Env::from_raw constructor

[jgraettinger]

Support usages where the caller already has an environment built by an
alternative means, which the Env of this crate should take ownership of.

[aleksuss]

Couple of questions.

1. Why ?
2. What is gonna be a caller of this method ?
3. Where are tests for this method ?
4. And we shouldn't forget about such case ?

[jgraettinger]

1. The rocks environment provides mechanisms to hook into lower-level subsystems of Rocks, such as file IO. For example the rocks gazette consumer store leverages this to replicate databases states as they're being written. You're deep into FFI land to do this type of thing, of course, but once you've built an instrumented FFI environment it's nice to hand it to this crate for usage.

2. Example call site. You have to already have an FFI *env pointer of course. This PR merely plumbs through a means to hand it to an Env of this crate, and as best I can tell matches the standard conventions for exposing and naming an unsafe operation of this kind in the standard library and elsewhere.

3. I'm not sure what it would cover, that the compiler doesn't check already. If there were a paired into_raw() function then a test case might exercise round-tripping through that raw pointer, but I don't get the feeling an into_raw is desired and I'm not suggesting it. (FWIW there's of course plenty of integrated, end-to-end coverage in our repo that uses this API).

4. That's why this function is unsafe, and specifically notes it's unsafe because ownership is transferred, similarly to how Vec::from_raw_parts and other functions of this kind are documented.

[stanislav-tkach]

I think it would be better to extend the "why it is unsafe" section with a better explanation of how this function should be used safely. I would prefer to follow the std approach in describing such things. Vec::set_len description can be used as an example.

[jgraettinger]

I think it would be better to extend the "why it is unsafe" section

After a short multi-year delay 😁 I've extended this section with more discussion.

[jgraettinger]

Bump @stanislav-tkach ? Thanks

[stanislav-tkach]

Thanks for the update, but I have already approved the pull request. 🙃

@zaidoon1 What do you think?

[aleksuss]

@jgraettinger could you please add any test or example of usage? But I don't like to expose unsafe public methods.

[jgraettinger]

@jgraettinger could you please add any test or example of usage? But I don't like to expose unsafe public methods.

🤔 I don't know how we'd realistically do that without dramatically blowing up the scope of this change. For example, it would require new FFI bridge code to get a raw Env out from this crate -- which would be silly, considering the motivating use case is having an Env that's wired up with C++ handlers that the C API doesn't even have access too.

Open to ideas.

Here's an actual call site in a real application: https://github.com/estuary/flow/blob/master/crates/runtime/src/rocksdb.rs#L236-L243

Here's an example why you would want something like this (instrumenting to capture file operations issued by the DB, as they're happening):
https://pkg.go.dev/go.gazette.dev/core@v0.89.0/consumer/store-rocksdb

An example docstring in this context could just be:

let mut opts = rocksdb::Options::default();

// Re-hydrate the provided memory address into rocksdb::Env wrapping
// an owned *mut librocksdb_sys::rocksdb_env_t.
let env = unsafe {
    rocksdb::Env::from_raw(rocksdb_env_memptr as *mut librocksdb_sys::rocksdb_env_t)
};
opts.set_env(&env);

Is this what you had in mind?

But I don't like to expose unsafe public methods.

I mean, this is a crate that's wrapping a highly customize-able, performance critical C++ library. It comes with the territory.