Add a subscription manager #548

HCastano · 2020-05-19T21:41:34Z

I was implementing some pubsub APIs in Substrate, outside of sc-rpc, and got to a point where I needed something to execute the subscription Futures. There's nothing provided out of the box for doing that, so after talking with @tomusdrw we figured that it might be useful to have the Substrate subscription manager be a part of the pubsub crate.

What I've done is I've stolen that code and changed it to try and be a little less opinionated. One concern I have though is that I've changed SubscriptionId to now take a generic parameter, N, to avoid only working with u64 ids, and I think this will break people's code.

I'm looking for feedback on whether the general approach is alright, and if the change to SubscriptionId is acceptable.

The idea is to use the `Subscriptions` struct from Substrate, which is used to drive subscription futures to completion, and modify it for "general" use.

tomusdrw · 2020-05-20T12:21:31Z

pubsub/src/manager.rs

+
+/// Trait used to provide unique subscription ids.
+pub trait IdProvider {
+	// TODO: Maybe have this impl Into<u64>?


It should be possible to have String ids as well.

tomusdrw · 2020-05-20T12:22:45Z

pubsub/src/manager.rs

+pub struct Manager<I: Default + IdProvider> {
+	next_id: I,
+	active_subscriptions: Arc<Mutex<HashMap<I::Id, oneshot::Sender<()>>>>,
+	executor: TaskExecutor, // Make generic?


IMHO It's fine, in most workloads spawning subscription is not going to be a bottleneck so we can safely do a virtual call here.

tomusdrw · 2020-05-20T12:24:35Z

pubsub/src/manager.rs

+}
+
+/// Trait used to drive subscription Futures to completion.
+pub trait SubscriptionManager {


I'm not sure the trait is really needed, I'd rather have Manager with a default generic parameter of u64 subscription IdProvider. Mainly to avoid importing extra traits to call functions.
Do you have some use cases in mind where having it behind trait would be useful?

Yeah, my reasoning kinda was that if people wanted a "default" implementation of a SubscriptionManager they'd use the Manager struct, and if they wanted to roll their own they just had to conform to the SubscriptionManager trait. Is it reasonable to expect that people will want to use something other than the "default" implementation? If not, then yeah we don't need to the trait.

Since the trait is not really used in the library at all I think a concrete implementation is enough. If someone is building their own solution and needs that abstraction they can introduce a trait themselves and implement it for the type from our library.

tomusdrw · 2020-05-20T12:27:30Z

pubsub/src/types.rs

-pub enum SubscriptionId {
-	/// U64 number
-	Number(u64),
+pub enum SubscriptionId<N> {


Ah, I see what you meant with Into<u64> in the earlier comment. Why not require Into<SubscriptionId> instead? This together with some extra From implementations that we would add here (for numeric types and String as you did now) would allow us to keep that configurable without maiking SubscriptionId generic.

Adds trait bounds that allow conversion between the two, removing the need for generics in SubscriptionId.

tomusdrw

Looks good, just needs some API tweaking and more docs.

tomusdrw · 2020-05-21T12:32:28Z

pubsub/src/manager.rs

@@ -0,0 +1,88 @@
+//! Provides an executor for subscription Futures.


I think we should expand on this module description a bit. The subscription manager is opinionated way to handle processing subscriptions that are based on Stream coming from the user codebase.
The manager takes care of:

Assigning the ID

Consuming a Stream of events (coming from user code) and transforming it into a subscription notifications to the client.

Spawning a resulting future to executor

Cancelling the stream when the subscription is canceled.

tomusdrw · 2020-05-21T12:34:04Z

pubsub/src/manager.rs

+	executor: TaskExecutor, // Make generic?
+}
+
+impl<I: Default + IdProvider> SubscriptionManager<I> {


Default should only be required for new (not for add/executor/etc) and there should be an extra method with_id_provider(TaskExecutor, IdProvider) for non-default cases.

pubsub/src/manager.rs

tomusdrw · 2020-05-21T12:40:55Z

pubsub/src/manager.rs

+/// Takes care of assigning unique subscription ids and
+/// driving the sinks into completion.
+#[derive(Clone)]
+pub struct SubscriptionManager<I: Default + IdProvider> {


Suggested change

pub struct SubscriptionManager<I: Default + IdProvider> {

pub struct SubscriptionManager<I: IdProvider = RandomizedStringProvider> {

We should implement two IdProviders:

NumericIdProvider

RandomizedStringProvider

The first one would simply use AtomicU64 and would return increasing numeric values, the second one - which should also be a default to use if SubscriptionManager does not include generic parameter - should return a randomized strings (guids).
The reason for using randomized strings is to make sure that subscription ids are not easy to guess, so that multiple clients can't cancel each-others subscriptions easily. The first one will be mostly usable for tests or cases where the aforementioned issue is not a problem.

tomusdrw · 2020-05-21T12:42:06Z

pubsub/src/types.rs

+	}
+
+	#[test]
+	fn should_convert_from_string_to_subscription_id() {


Thanks for all the test ❤️

Adds two subscription ID providers which can be used by the SubscriptionManager. One provides a simple numeric ID, while the other provides a random string.

tomusdrw

Looks good! Few tiny things left.

tomusdrw · 2020-05-22T08:02:21Z

pubsub/src/manager.rs

+		let id: String = iter::repeat(())
+			.map(|()| rng.sample(Alphanumeric))
+			.take(self.len)
+			.collect();


Nice and elegant :)

Unfortunately I can't take credit for it: https://rust-random.github.io/rand/rand/distributions/struct.Alphanumeric.html

tomusdrw · 2020-05-22T08:02:42Z

pubsub/src/manager.rs

-	active_subscriptions: Arc<Mutex<HashMap<I::Id, oneshot::Sender<()>>>>,
+pub struct SubscriptionManager<I: IdProvider = RandomStringIdProvider> {
+	id_provider: I,
+	active_subscriptions: ActiveSubscriptions,
 	executor: TaskExecutor, // Make generic?


Suggested change

executor: TaskExecutor, // Make generic?

executor: TaskExecutor,

tomusdrw · 2020-05-22T08:03:34Z

pubsub/src/manager.rs

 	/// Creates a new SubscriptionManager.
-	pub fn new(executor: TaskExecutor) -> Self {
+	pub fn new_with_id_provider(id_provider: I, executor: TaskExecutor) -> Self {


Suggested change

pub fn new_with_id_provider(id_provider: I, executor: TaskExecutor) -> Self {

pub fn with_id_provider(id_provider: I, executor: TaskExecutor) -> Self {

AFAIK the overall convention is to skip new_ in with_ constructors.

tomusdrw · 2020-05-22T08:06:23Z

pubsub/src/types.rs

@@ -62,6 +62,14 @@ impl From<String> for SubscriptionId {
 	}
 }

+// TODO: Don't unwrap, and probably implement TryFrom instead of From
+use std::convert::TryInto;
+impl From<usize> for SubscriptionId {


I think instead of providing this From<usize> implementation we should rather change the NumericIdProvider to convert usize to u64 internally. The reason is that it's highly unlikely for NumericIdProvider to reach the case where this conversion might fail (given it runs on 128-bit processor anyway O.o) and here we are making stronger assumption that usize can be safely converted to SubscriptionId.

Instead of providing a guarantee that we can convert between `usize` and `u64` we make the assumptions that it's unlikely that we're running on an architecture larger than 64-bits and we use a `u64` directly.

HCastano · 2020-05-22T23:00:39Z

I want to write some tests to make sure the SubscriptionManager can add() and cancel() subscriptions before I mark this as "ready for review" (even though you've already reviewed it @tomusdrw , lol).

Note: There's one test that doesn't pass yet which has to do with the `cancel()` function of the SubscriptionManager.

HCastano · 2020-05-27T16:05:49Z

Looks like the Windows runner is failing due to an old rustc version which doesn't have matches! stabilized yet

Our Windows CI runner isn't up to date and thinks this is still a nightly feature

WIP: Add a subscription manager.

7ef90a3

The idea is to use the `Subscriptions` struct from Substrate, which is used to drive subscription futures to completion, and modify it for "general" use.

This comment has been minimized.

Sign in to view

HCastano added the inprogress label May 19, 2020

tomusdrw reviewed May 20, 2020

View reviewed changes

HCastano added 3 commits May 20, 2020 20:57

Allow IdProvider::Id and SubscriptionId to work together

9883720

Adds trait bounds that allow conversion between the two, removing the need for generics in SubscriptionId.

Update SubscriptionId tests

a41638e

Rustfmt

05b1a92

tomusdrw reviewed May 21, 2020

View reviewed changes

HCastano added 3 commits May 21, 2020 16:51

Use SubscriptionId as the key for active_subscriptions

0149fd4

Add subscription ID providers.

f0fabd9

Adds two subscription ID providers which can be used by the SubscriptionManager. One provides a simple numeric ID, while the other provides a random string.

Add some documentation

c89511d

tomusdrw approved these changes May 22, 2020

View reviewed changes

HCastano added 2 commits May 22, 2020 10:53

Clean up comment and naming

cd31378

Change the NumericIdProvider to use u64 IDs

3cf3113

Instead of providing a guarantee that we can convert between `usize` and `u64` we make the assumptions that it's unlikely that we're running on an architecture larger than 64-bits and we use a `u64` directly.

HCastano mentioned this pull request May 22, 2020

Add Subscription RPC for Grandpa Finality paritytech/substrate#5732

Merged

HCastano and others added 6 commits May 25, 2020 18:40

Add tests for IdProvider and SubscriptionManager

7e435a0

Note: There's one test that doesn't pass yet which has to do with the `cancel()` function of the SubscriptionManager.

Restore RandomStringIdProvider as the default provider

1e5846f

Retain receiver.:

1334760

Make test executor a lazy static

476edee

Rustfmt

c3dafbc

Add a comment to test

52caa9a

HCastano marked this pull request as ready for review May 27, 2020 15:56

HCastano requested a review from tomusdrw May 27, 2020 15:57

Remove matches! macro

2c27a2d

Our Windows CI runner isn't up to date and thinks this is still a nightly feature

tomusdrw approved these changes May 27, 2020

View reviewed changes

tomusdrw merged commit 1779dbe into paritytech:master May 27, 2020

HCastano deleted the hc-add-subscription-manager branch May 27, 2020 16:37

HCastano mentioned this pull request Jun 1, 2020

Use Subscription Manager from jsonrpc-pubsub paritytech/substrate#6208

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a subscription manager #548

Add a subscription manager #548

HCastano commented May 19, 2020

This comment has been minimized.

tomusdrw May 20, 2020

tomusdrw May 20, 2020

tomusdrw May 20, 2020

HCastano May 20, 2020

tomusdrw May 20, 2020

tomusdrw May 20, 2020

tomusdrw left a comment

tomusdrw May 21, 2020

tomusdrw May 21, 2020

tomusdrw May 21, 2020

tomusdrw May 21, 2020

tomusdrw left a comment

tomusdrw May 22, 2020

HCastano May 22, 2020

tomusdrw May 22, 2020

tomusdrw May 22, 2020

tomusdrw May 22, 2020

HCastano commented May 22, 2020

HCastano commented May 27, 2020

		@@ -0,0 +1,88 @@
		//! Provides an executor for subscription Futures.

	pub struct SubscriptionManager<I: Default + IdProvider> {
	pub struct SubscriptionManager<I: IdProvider = RandomizedStringProvider> {

	executor: TaskExecutor, // Make generic?
	executor: TaskExecutor,

	pub fn new_with_id_provider(id_provider: I, executor: TaskExecutor) -> Self {
	pub fn with_id_provider(id_provider: I, executor: TaskExecutor) -> Self {

Add a subscription manager #548

Add a subscription manager #548

Conversation

HCastano commented May 19, 2020

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomusdrw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomusdrw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HCastano commented May 22, 2020

HCastano commented May 27, 2020