Hide RaftStorage and similar types from generic arguments to Raft/RaftInner

[schreter]

Currently, the types for storage, log and network factory are passed as generic arguments to Raft. This means that the Send and Sync traits of Raft/RaftInner are derived from these types as well.

For single-threaded Raft instances, this means that the Raft object would be !Send and !Sync, even though these types are only used in the single task where RaftInner is running. In the current implementation (see also PR #934), the Send and Sync traits are explicitly implemented for single-threaded Raft when the data types actually exchanged between threads (such as request and response) are Send. However, such implementation must be marked unsafe.

Ideally, deriving Send and Sync traits should be left to the compiler.

To fix this, we need to convert generic arguments to associated types. Then, only the respective usage of the associated type which is !Send would prevent cross-thread communication (i.e., within the Raft task - where we do want to have it single-threaded).

Other types (like request/response) are passed via RaftTypeConfig already. To pass storage, log and network factory types, we can either devise a new config trait, such as RaftStorageConfig, where these types would be defined as associated types, or, we can simply add them to RaftTypeConfig. In either case, the containing trait instance is as such Send + Sync, only (some of) the associated types aren't. In this way, we don't need the unsafe impl of the two traits.

The question is, which way to go (single RaftTypeConfig or two traits). In any case, this is a breaking change to the API.

For the latter case (two config traits), the API change can be hidden by forwarding from the old API to the new API via a suitably-written type alias and reexports and using a feature switch to enable this forwarding for old users.

Ideas? Suggestions?

[github-actions]

👋 Thanks for opening this issue!

Get help or engage by:

• /help : to print help messages.
• /assignme : to assign this issue to you.

[schreter]

@drmingdrmer @wvwwvwwv

[drmingdrmer]

The question is, which way to go (single RaftTypeConfig or two traits). In any case, this is a breaking change to the API.

Adding Network, LogStore, and StateMachine into RaftTypeConfig could lead to problems with cyclic references. We previously encountered issues in an unsuccessful attempt:

• Change: add 'RaftStorage' and 'RaftNetwork' into 'RaftTypeConfig' #372

[schreter]

Adding Network, LogStore, and StateMachine into RaftTypeConfig could lead to problems with cyclic references. We previously encountered issues in an unsuccessful attempt.

Right... So a second trait? It also makes the upgrade path much simpler for existing users.

[drmingdrmer]

A second trait would be fine to me. :)

[drmingdrmer]

@schreter
Can this issue be closed now?

[schreter]

Can this issue be closed now?

Yes, of course, now that the generics are gone :-).