Gate

[patjed41]

Fixes: #4 Depends on: #1, #2, #13

This PR introduces exposition of seastar::gate to Rust as struct Gate with methods:

pub fn new() -> Pin<Box<Self>>;
pub fn try_enter(&'a self) -> Result<GateHolder<'a>, GateClosedError>;
pub async fn close(&self);

There are also 2 auxiliary types:

• GateHolder<'a> – equivalent of seastar::gate::holder,
• GateClosedError – equivalent of seastar::gate_closed_exception.

Currently, there are 3 tests testing the following scenarios:

• close,
• try_enter -> leave -> close,
• close -> try_enter (GateClosedError is returned).

A very important case try_enter -> close -> leave (close waits) hasn't been tested yet, because it requires Seastar runtime. This particular case (and maybe some other cases with more enters) will be tested when Seastar runtime is exposed to Rust.

There are some issues with this PR that I have noticed:

• GateHolder holds reference to gate just to implement the lifetime. It would be really nice if something like this

  pub struct GateHolder<'a> {
    _holder: 'a UniquePtr<gate_holder>,
  }

  worked, but unfortunately it doesn't. I haven't found a way to implement GateHolder without keeping &'a gate.

• Our fork of cxx-async is not usable yet, because PR hasn't been accepted there. Instead, I used fork of our fork.
• cxx-async throws many warnings.
• CloseGateFuture appears in the documentation generated by cargo doc and I don't know how to hide it. This

  #[doc(hidden)]
  type CloseGateFuture = crate::CloseGateFuture;

  doesn't work.

• GateClosedError has a hard coded implemantation of the Debug trait. It should be possible to print seastar::gate_closed_exception::what() somehow. I'll do it if it is necessarry.

[patjed41]

I was worried about CloseGateFuture appearing in the documentation but the problem here is that it's public. Change like this

type CloseGateFuture = super::CloseGateFuture;

plus changing

pub use gate::*;

to

pub use gate::{GateClosedError, Gate, GateHolder};

in lib.rs solves it. But still, it feels bad to specify all types publicly imported from a module in lib.rs just to avoid exposition of a single type used by cxx-async. I would appreciate some suggestions about this issue.

[piodul]

This PR introduces exposition of seastar::gate to Rust as struct Gate with methods:

pub fn new() -> Pin<Box<Self>>;
pub fn try_enter(&'a self) -> Result<GateHolder<'a>, GateClosedError>;
pub async fn close(&self);

There are also 2 auxiliary types:

• GateHolder<'a> – equivalent of seastar::gate::holder,
• GateClosedError – equivalent of seastar::gate_closed_exception.

Currently, there are 3 tests testing the following scenarios:

• close,
• try_enter -> leave -> close,
• close -> try_enter (GateClosedError is returned).

A very important case try_enter -> close -> leave (close waits) hasn't been tested yet, because it requires Seastar runtime. This particular case (and maybe some other cases with more enters) will be tested when Seastar runtime is exposed to Rust.

There are some issues with this PR that I have noticed:

• GateHolder holds reference to gate just to implement the lifetime. It would be really nice if something like this

  pub struct GateHolder<'a> {
    _holder: 'a UniquePtr<gate_holder>,
  }

  worked, but unfortunately it doesn't. I haven't found a way to implement GateHolder without keeping &'a gate.

You can use PhantomData. It is very helpful when you want to implement a generic type that has a type or lifetime parameter but is actually not necessary in the struct definition.

Here, I think it should look something like this:

use std::marker::PhantomData;
pub struct GateHolder<'a> {
    _holder: UniquePtr<gate_holder>,
    _phantom: PhantomData<&'a ()>,
}

• Our fork of cxx-async is not usable yet, because PR hasn't been accepted there. Instead, I used fork of our fork.
• cxx-async throws many warnings.

• CloseGateFuture appears in the documentation generated by cargo doc and I don't know how to hide it. This

  #[doc(hidden)]
  type CloseGateFuture = crate::CloseGateFuture;

  doesn't work.

I suppose that the #[doc(hidden)] parameter should appear next to where the original CloseGateFuture is defined. I'm not sure whether this is possible to do, it probably depends on how the cxx_async macro was implemented. Maybe you can put it before unsafe impl Future for CloseGateFuture?

• GateClosedError has a hard coded implemantation of the Debug trait. It should be possible to print seastar::gate_closed_exception::what() somehow. I'll do it if it is necessarry.

I suggest adding a dependency on the thiserror crate. It makes generating such error types very easy.

---

Last thing: if you send a PR that should close an issue, please add the following line to the PR description:

Fixes: #issue_number

[piodul]

I was worried about CloseGateFuture appearing in the documentation but the problem here is that it's public. Change like this

type CloseGateFuture = super::CloseGateFuture;

plus changing

pub use gate::*;

to

pub use gate::{GateClosedError, Gate, GateHolder};

in lib.rs solves it. But still, it feels bad to specify all types publicly imported from a module in lib.rs just to avoid exposition of a single type used by cxx-async. I would appreciate some suggestions about this issue.

I suggest introducing an internal module just for cxx_async's futures. Their names would correspond to the types that they return, so it won't feel as awkward to define it in a different place than they are used. Moreover, such a single type could be reused in multiple places.

[patjed41]

You can use PhantomData. It is very helpful when you want to implement a generic type that has a type or lifetime parameter but is actually not necessary in the struct definition.

It worked nicely.

I suggest adding a dependency on the thiserror crate. It makes generating such error types very easy.

I used it. Putting #[error("GateClosedError: gate closed")] before the GateClosedError declaration sets the Display message instead of the Debug one. I guess it's fine?

I suggest introducing an internal module just for cxx_async's futures. Their names would correspond to the types that they return, so it won't feel as awkward to define it in a different place than they are used. Moreover, such a single type could be reused in multiple places.

Done. That was a very good idea. It solved all the issues at the same time.

[patjed41]

I've made some changes:

• C++ side of the ffi uses seastar_ffi::gate namespace,
• tests use seastar::test macro,
• there is a new test that checks the case try_enter -> close -> leave (there was a segfault without the runtime),
• Gate::close checks if the runtime is running using assert_runtime_is_running! macro.

I think that this task is finished now and it is only waiting for a review. If tests are not strong enough, I can think of writing more. The problem is that I don't know how to do it reasonably. Spawning tasks that would leave the gate after some time could be a solution, but #5 hasn't been finished yet.

[patjed41]

Update:

• Made changes requested in a review.
• Added more verbose commit messages to commits where it seemed reasonable.
• Split Add Gate::new commit to 2 commits: Create gate module and Add Gate with its constructor. I thought that this commit did too many things.
• Moved cherry-picking API safety to a separate commit.
• Changed some names to better match Rust conventions:
  • Gate::gate -> Gate::inner,
  • GateHolder::_holder -> GateHolder::_inner.

[patjed41]

Small update:

• I noticed that lifetimes in Gate::try_enter can be elided and I did that.
• I fixed an unused import warning in submit_to.rs (last commit). I think this PR will be merged first so it looks like a good place for a fix like that.