Websockets Support #49

benjamincburns · 2018-01-08T23:00:29Z

This PR supersedes #14, as it is based on those changes. Thanks heaps to @perissology for his submission!

How this all works:

Subscription creation and notification:

In Add subscription subprovider to handle websocket pub/sub MetaMask/web3-provider-engine#207, I created a SubscriptionSubprovider which extends the existing FilterSubprovider
When the SubscriptionSubprovider gets an eth_subscribe request, it uses the underlying FilterSubprovider code to create a Filter (i.e. LogFilter, BlockFilter, or PendingBlockFilter) for the subscription, then returns the id of this filter.
I refactored the LogFilter, BlockFilter and PendingBlockFilter types to emit a data event on update
When SubscriptionSubprovider creates a filter, it subscribes to the filter's data event, and and emits its own data event when the Filter's event fires, clearing pending filter changes in the process.
Similarly, I refactored our Provider type to be an EventEmitter and to proxy data events from the SubscriptionSubprovider.

Routing notifications to clients:

As one might expect, we listen on a WebSocket, and proxy RPC calls over to the existing Provider type.
The component responsible for this proxying is the ConnectionManager.
The ConnectionManager snoops on the traffic it's proxying to maintain a forward and reverse mapping of subscription ID to websocket connection.
The ConnectionManager listens to data events from the Provider, and uses its mapping of subscription ID to connection to route the subscription notifications to the correct websocket connection.
When an eth_unsubscribe request is handled, ConnectionManager cleans up the relevant mapping.
When a client disconnects, ConnectionManager cleans up all mappings pertaining to that client.

TODO:

Fix intermittent forking test failure
Fix block tracker to avoid polling
Rebase on current develop
Squash a bunch of noisy commits (especially the one where I misspelled "concurrency")

gnidan

Hey @benjamincburns - couple comments / thoughts from a first read.

Looks great! Lots of bits and pieces moving around, but generally seems pretty intuitive.

Hat tip 👒 to @perissology for getting this started!

gnidan · 2018-01-12T18:24:13Z

lib/blockchain_double.js

+        self.emit('block', block);
+      }
+      callback(err, result);
+    });


Is there risk of blocks being emitted out of order? Would that matter?

There shouldn't be. pushBlock is only called when processBlock wants to commit a new block to the chain, when the genesis block is created, or on start with an existing db when latest is read from that db. None of those things should be possible to order incorrectly.

gnidan · 2018-01-12T18:39:03Z

lib/server.js

+
+    if (options.ws) {
+      wss = webSocketServer(server, provider, logger);
+    }


Should this get closed when the httpServer closes?

I thought so at first as well, but after testing, I think this is the correct way to do it. webSocketServer has a reference to the httpServer, meaning it listens on the same socket, and its connections are tracked/managed by the httpServer anyway.

gnidan · 2018-01-12T18:40:30Z

lib/statemanager.js

@@ -342,13 +336,12 @@ StateManager.prototype.queueTransaction = function(method, tx_params, callback)
    self.action_queue.push({
      method: method,
      from: tx_params.from,
-      tx: tx,
+      tx: rawTx,
      callback: callback
    });

    // We know there's work, so get started.
    self.processNextAction();


Indent (L322-344)

gnidan · 2018-01-12T18:43:02Z

lib/statemanager.js

@@ -319,12 +319,6 @@ StateManager.prototype.queueTransaction = function(method, tx_params, callback)
  // Get the nonce for this address, taking account any transactions already queued.


Comment is out of date if you're moving nonce stuff.

gnidan · 2018-01-12T18:47:10Z

lib/statemanager.js


-  var tx_hash = to.hex(tx.hash());
+    let tx = new FakeTransaction(rawTx);


Possible suggestion: this getQueuedNonce logic is now repeated 3 times - maybe worth abstracting into a prepareTx function, or somesuch. Might make processCall/processGasEstimate/processTransaction a lot easier to read.

definitely.

gnidan · 2018-01-12T18:58:02Z

lib/webSocketServer.js

+  self.connections[ connection.id ] = {
+    connection: connection,
+    subscriptions: [],
+    subscriptionCounter: 0,


Seems like this is unused

I'll double check, I think you're correct. I think it's likely hold over from the initial set of refactored changes.

I didn't get to this yesterday. Will fix this now.

…t failures

…on contract deployments

tcoulter · 2018-01-15T23:26:15Z

test/runtime_errors.js

+  after('shutdown', function(done) {
+    let provider = web3._provider;
+    web3.setProvider();
+    provider.close(done);


What does web3.setProvider() do when not passing in a provider as a parameter?

from memory it sets the provider to undefined (it doesn't check the arg). I think I was being overly pedantic here though. My thought process was that web3 has a bunch of stuff it does in the background, and I didn't want it trying to run a request during or after provider close.

Further, I think calling provider.close is a good idea in general when using the direct provider (probably need to make sure other tests do this), but unfortunately it's not a standard part of the interface, so I need to resort to something like provider.connection.close for the WebSocket provider. Maybe I'll raise an issue for that.

benjamincburns self-assigned this Jan 8, 2018

This was referenced Jan 8, 2018

Add Websocket support #14

Closed

Add Web3 1.0 support MetaMask/metamask-extension#2350

Closed

benjamincburns force-pushed the websockets-pr-redux branch from d05ab71 to d3c0181 Compare January 12, 2018 12:42

Initial websocket support

2b1e1f5

benjamincburns force-pushed the websockets-pr-redux branch 5 times, most recently from d758778 to 2dc89a1 Compare January 12, 2018 13:02

rework websocket support from @perissology and update tests

9f83342

benjamincburns force-pushed the websockets-pr-redux branch from 2dc89a1 to 9f83342 Compare January 12, 2018 13:07

benjamincburns changed the title ~~WIP: Websockets Redux~~ Websockets Support Jan 12, 2018

This was referenced Jan 12, 2018

Logsbloom on tx receipt #52

Merged

Queue requests #53

Merged

gnidan reviewed Jan 12, 2018

View reviewed changes

benjamincburns added 4 commits January 15, 2018 18:10

statemanager whitespace

b2c4e0e

Clean up nonce calculation logic in statemanager

7ab917b

Make mining_start mining_stop test more robust to prevent intermitten…

49ee4ae

…t failures

Add old DelayedBlockFilter subprovider so that old web3 doesn't hang …

1ea7a62

…on contract deployments

tcoulter reviewed Jan 15, 2018

View reviewed changes

remove unused field, and make subscription routing use O(1) lookups

0a97471

benjamincburns merged commit 0a97471 into develop Jan 15, 2018

benjamincburns mentioned this pull request Jan 16, 2018

Support WebSocket as a transport for the RPC. trufflesuite/ganache-cli-archive#257

Closed

apanizo mentioned this pull request Feb 28, 2018

Handling Metamask's lack of websocket support/Web3 1.0 trufflesuite/drizzle-legacy#10

Closed

mikeseese deleted the websockets-pr-redux branch October 16, 2018 19:27

benjamincburns removed their assignment Jul 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Websockets Support #49

Websockets Support #49

benjamincburns commented Jan 8, 2018 •

edited

Loading

gnidan left a comment

gnidan Jan 12, 2018

benjamincburns Jan 15, 2018

gnidan Jan 12, 2018

benjamincburns Jan 15, 2018

gnidan Jan 12, 2018

benjamincburns Jan 15, 2018

gnidan Jan 12, 2018

benjamincburns Jan 15, 2018

gnidan Jan 12, 2018

benjamincburns Jan 15, 2018

gnidan Jan 12, 2018

benjamincburns Jan 15, 2018

benjamincburns Jan 15, 2018

tcoulter Jan 15, 2018

benjamincburns Jan 15, 2018

		@@ -319,12 +319,6 @@ StateManager.prototype.queueTransaction = function(method, tx_params, callback)
		// Get the nonce for this address, taking account any transactions already queued.


		var tx_hash = to.hex(tx.hash());
		let tx = new FakeTransaction(rawTx);

Websockets Support #49

Websockets Support #49

Conversation

benjamincburns commented Jan 8, 2018 • edited Loading

gnidan 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

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

benjamincburns commented Jan 8, 2018 •

edited

Loading