Pub sub blocks #6139

CraigglesO · 2017-07-25T02:00:09Z

Not entirely sure I am using unsubscribe properly, would like you to verify,
I changed localTransactions, if that causes any problems let me know, just needed more control.

…ocks

rphmeier · 2017-07-25T08:16:05Z

rpc/src/v1/impls/parity.rs

@@ -79,7 +79,7 @@ impl<C, M, S: ?Sized, U> ParityClient<C, M, S, U> where
 	U: UpdateService,
 {
 	/// Creates new `ParityClient`.
-	pub fn new(
+	pub fn new (


please revert this change

kaikun213

Looks good to me. You can also save the promise and then unsubscribe with resolving the promise. Otherwise it can happen that you unsubscribe before even having the subscriptionId, which would be invalid and have to be repeated.

I'm not sure where local transactions are all used but seems fine @jacogr .

tomusdrw

Couple grumbles.

tomusdrw · 2017-08-02T08:06:33Z

Cargo.lock

@@ -418,6 +418,14 @@ dependencies = [
 ]

 [[package]]
+name = "error-chain"


Seems like those changes are unnecessary.

tomusdrw · 2017-08-02T08:08:42Z

js/src/views/Status/NodeStatus/store.js

  }

  stopPolling () {
-    Object.keys(this._timeoutIds).forEach((key) => clearTimeout(this._timeoutIds[key]));
+    if (this.subscriptionId) {


Should wait for subscriptionId, otherwise if you stopPolling before the promises in startPolling are resolved you may end up with dangling subscription.

tomusdrw · 2017-08-02T08:09:19Z

js/src/views/Signer/store.js

@@ -49,8 +49,8 @@ export default class SignerStore {
  }

  @action unsubscribe () {
-    if (this._timeoutId) {
-      clearTimeout(this._timeoutId);
+    if (this.subscriptionId) {


It should always wait for subscription id, otherwise you may end up with a dangling subscription.

tomusdrw · 2017-08-02T08:10:28Z

js/src/views/Signer/store.js

@@ -88,20 +88,18 @@ export default class SignerStore {
  }

  fetchLocalTransactions = () => {


The name suggests that the transactions will be fetched, but actually it sets up a subscription.
The name should be altered and it should be ensured that the method is never called more than once.

tomusdrw · 2017-08-02T08:14:06Z

js/src/views/Signer/store.js

+    })
+    .then((subscriptionId) => {
+      this.subscriptionId = subscriptionId;
+    });


Would be good to at least log errors here.

tomusdrw · 2017-08-02T08:14:11Z

js/src/views/Status/NodeStatus/store.js

+      this.blockNumber = block.number;
+      this.blockTimestamp = block.timestamp;
+      this._pollMinerSettings();
+      return Promise


return here is misleading, it looks a bit like then(subscriptionId) is chained to this promise, but it's not.

tomusdrw · 2017-08-02T08:14:23Z

js/src/views/Status/NodeStatus/store.js

+    })
+    .then((subscriptionId) => {
+      this.subscriptionId = subscriptionId;
+    });


Would be good to at least log errors.

CraigglesO · 2017-08-06T15:45:11Z

cleaned up and handles all errors/use cases. Lemme know what you think.

kaikun213 · 2017-08-07T09:22:07Z

js/src/views/Signer/store.js

+    const self = this;
+
+    if (self.subscribed) {
+      if (!self.subscriptionId) {


You can simplify the code by saving the promise itself as subscription and then resolving it in unsubscribe.
This works fine when the id does not need to get exposed.
E.g. like here in the bonds library

Ah, I understand your design now and how nice that is. Sorry I didn't fully understand your first comment.

gavofyork · 2017-08-29T12:38:36Z

@tomusdrw could you take a look at this?

ngotchac · 2017-08-30T15:06:49Z

js/src/api/pubsub/parity/parity.js

-            });
-          return transactions;
-        });
+        : callback(null, transactions);


Why do we get rid of the outTransaction here?

Good catch. Brought it back.

tomusdrw · 2017-08-31T08:39:43Z

@ngotchac Fixed, please have a look.

ngotchac

LGTM. I just think that the NodeStatus/store should be a singleton, instanciated once. But this was introduced in another PR so...

CraigglesO added 4 commits July 23, 2017 23:39

fun start

781a1ca

Merge branch 'master' of github.com:paritytech/parity into pub-sub-bl…

22f9582

…ocks

playin

7067ba6

linting

961e264

CraigglesO requested review from jacogr and kaikun213 July 25, 2017 02:00

CraigglesO added 3 commits July 24, 2017 20:04

cleanup kk 1

abae7d7

cleanup kk 2

3f69243

package

ccfa8a3

gavofyork added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. M7-ui and removed M4-core ⛓ Core client code / Rust. labels Jul 25, 2017

rphmeier reviewed Jul 25, 2017

View reviewed changes

kaikun213 reviewed Jul 25, 2017

View reviewed changes

oops

5760903

tomusdrw reviewed Aug 2, 2017

View reviewed changes

tomusdrw added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 2, 2017

CraigglesO added 6 commits August 3, 2017 02:29

grumbles

86a89bb

lint

22257cb

lint 2

5334996

all-possible-cases

78ffe02

conflict resolution

38059f8

conflict resolution

b3b939b

kaikun213 reviewed Aug 7, 2017

View reviewed changes

CraigglesO added 3 commits August 7, 2017 07:09

ready

59a1f54

lint

bfb15eb

remove package-lock

927b4c8

CraigglesO added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Aug 7, 2017

tomusdrw added 2 commits August 30, 2017 15:14

Restart subscription on transport close.

cceda15

Merge branch 'master' into pub-sub-blocks

9e6b7ab

tomusdrw added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 30, 2017

tomusdrw approved these changes Aug 30, 2017

View reviewed changes

ngotchac reviewed Aug 30, 2017

View reviewed changes

tomusdrw added 2 commits August 30, 2017 18:39

Bring back the formatter.

259c4bb

Fix formatter.

09c72a6

ngotchac approved these changes Sep 1, 2017

View reviewed changes

tomusdrw merged commit 56f46ed into master Sep 1, 2017

tomusdrw deleted the pub-sub-blocks branch September 1, 2017 10:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pub sub blocks #6139

Pub sub blocks #6139

CraigglesO commented Jul 25, 2017 •

edited

Loading

rphmeier Jul 25, 2017

kaikun213 left a comment

tomusdrw left a comment

tomusdrw Aug 2, 2017

tomusdrw Aug 2, 2017

tomusdrw Aug 2, 2017

tomusdrw Aug 2, 2017

tomusdrw Aug 2, 2017

tomusdrw Aug 2, 2017

tomusdrw Aug 2, 2017

CraigglesO commented Aug 6, 2017

kaikun213 Aug 7, 2017

CraigglesO Aug 7, 2017

gavofyork commented Aug 29, 2017

ngotchac Aug 30, 2017

tomusdrw Aug 30, 2017

tomusdrw commented Aug 31, 2017

ngotchac left a comment

		@@ -88,20 +88,18 @@ export default class SignerStore {
		}

		fetchLocalTransactions = () => {

Pub sub blocks #6139

Pub sub blocks #6139

Conversation

CraigglesO commented Jul 25, 2017 • edited Loading

Choose a reason for hiding this comment

kaikun213 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CraigglesO commented Aug 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gavofyork commented Aug 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomusdrw commented Aug 31, 2017

ngotchac left a comment

Choose a reason for hiding this comment

CraigglesO commented Jul 25, 2017 •

edited

Loading