Websockets support #458

mushketyk · 2018-03-28T21:45:54Z

Implements WebSocketService that allows calling JSON-RPC methods via WebSocket protocol and allows to subscribe to real-time notifications.
Also adds new methods to the Web3jRx that allows subscribing to JSON-RPC notifications.

The PR is already too big for a review, so I had to stop, but there are few things that are still missing:

Support for Parity pub-sub method. Parity allows subscribing to more notifications than Geth and PR allows using Parity's pub-sub mechanism
Integration tests for WebSockets
User documentation (I write it once this PR is approved, since we can change the design during the review)

codecov · 2018-03-28T22:04:54Z

Codecov Report

Merging #458 into master will increase coverage by 0.23%.
The diff coverage is 78.62%.

@@             Coverage Diff             @@
##             master    #458      +/-   ##
===========================================
+ Coverage     76.87%   77.1%   +0.23%     
- Complexity     1686    1770      +84     
===========================================
  Files           222     236      +14     
  Lines          6295    6581     +286     
  Branches        972     984      +12     
===========================================
+ Hits           4839    5074     +235     
- Misses         1215    1262      +47     
- Partials        241     245       +4

Impacted Files	Coverage Δ	Complexity Δ
...th/src/main/java/org/web3j/protocol/geth/Geth.java	`100% <ø> (ø)`	`1 <0> (ø)`	⬇️
.../java/org/web3j/protocol/websocket/events/Log.java	`0% <0%> (ø)`	`0 <0> (?)`
...protocol/core/methods/response/EthUnsubscribe.java	`0% <0%> (ø)`	`0 <0> (?)`
.../protocol/websocket/events/SyncingNotfication.java	`0% <0%> (ø)`	`0 <0> (?)`
...bsocket/events/PendingTransactionNotification.java	`0% <0%> (ø)`	`0 <0> (?)`
...b3j/protocol/websocket/events/LogNotification.java	`0% <0%> (ø)`	`0 <0> (?)`
...main/java/org/web3j/protocol/http/HttpService.java	`83.33% <0%> (+3.33%)`	`17 <0> (ø)`	⬇️
...rotocol/websocket/events/NewHeadsNotification.java	`100% <100%> (ø)`	`1 <1> (?)`
core/src/main/java/org/web3j/protocol/Service.java	`84.61% <100%> (+4.61%)`	`4 <1> (+1)`	⬆️
...org/web3j/protocol/websocket/WebSocketRequest.java	`100% <100%> (ø)`	`3 <3> (?)`
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f477549...e574576. Read the comment docs.

mushketyk · 2018-03-30T10:05:21Z

@conor10 I've made few more additional modifications to my PR, but now everything is ready for review.

conor10 · 2018-04-01T16:42:40Z

Awesome work @mushketyk 💯 - I'll get to it shortly.

mushketyk · 2018-04-03T21:04:30Z

Thank you @conor10! I look forward to your review.

conor10

Great work so far, apologies for the delay in reviewing.

conor10 · 2018-04-05T09:22:00Z

core/build.gradle

    testCompile project(path: ':crypto', configuration: 'archives'),
-            "nl.jqno.equalsverifier:equalsverifier:$equalsverifierVersion"
+            "nl.jqno.equalsverifier:equalsverifier:$equalsverifierVersion",
+            group: 'ch.qos.logback', name: 'logback-classic', version: '1.2.3'


Please can this dependency be included using the same style as the others?

conor10 · 2018-04-05T09:22:41Z

core/src/main/java/org/web3j/protocol/Web3jService.java

+     * @param <T> type of a data item returned by the request
+     * @return deserialized JSON-RPC response
+     * @throws IOException thrown if failed to perform a request
+     */


conor10 · 2018-04-15T13:07:29Z

core/src/main/java/org/web3j/protocol/rx/Web3jRx.java

+     *
+     * @return Observable that emits a notification for every new header
+     */
+    Observable<NewHeadsNotification> newHeadsNotifications();


This is Geth specific right? And some of the other calls?

newHeads and logs are not specific to Geth. Parity supports it as well: https://wiki.parity.io/JSONRPC-Eth-Pub-Sub-Module.html

I'll move other two to the Geth related class

conor10 · 2018-04-15T13:08:13Z

core/src/main/java/org/web3j/protocol/websocket/WebSocketClient.java

+        try {
+            log.debug("Received message {} from server {}", s, uri);
+            listener.onMessage(s);
+            log.debug("Processed message {} from server {}", s, uri);


I don't think we need both debug statements

Removed second message.

conor10 · 2018-04-15T13:10:11Z

core/src/main/java/org/web3j/protocol/websocket/WebSocketService.java

+
+            @Override
+            public void onError(Exception e) {
+


Please can we have a log statement here?

Do we need it here?

We have a log in the WebSocketClient class:

@Override public void onError(Exception e) { log.error(String.format("WebSocket connection to {} failed with error", uri), e); listener.onError(e); }

conor10 · 2018-04-15T13:10:50Z

core/src/main/java/org/web3j/protocol/websocket/WebSocketRequest.java

+        this.responseType = responseType;
+    }
+
+    public CompletableFuture<T> getCompletableFuture() {


Please can you use something more specific for this method name & instance variable?

Done. What do you think about onReply?

conor10 · 2018-04-15T13:15:26Z

core/src/main/java/org/web3j/protocol/websocket/WebSocketSubscription.java

+
+import rx.subjects.BehaviorSubject;
+
+public class WebSocketSubscription<T> {


Please can you ensure all classes have a brief Javadoc description - a single sentence is fine.

conor10 · 2018-04-15T13:16:00Z

core/src/main/java/org/web3j/protocol/websocket/events/Log.java

+
+import java.util.List;
+
+public class Log {


I presume this Log differs from the other Log objects already defined?

Yes, existing Log class contains the following fields:

private boolean removed; private String logIndex; private String transactionIndex; private String transactionHash; private String blockHash; private String blockNumber; private String address; private String data; private String type; private List<String> topics;

while this Log class contains the following fields:

private String address; private String blockHash; private String blockNumber; private String data; private String logIndex; private List<String> topics; private String transactionHash; private String transactionIndex;

conor10 · 2018-04-15T13:17:58Z

core/src/test/java/org/web3j/protocol/core/JsonRpc2_0WebSocketClientJTest.java

+    private Web3j web3j = Web3j.build(service, 10, scheduledExecutorService);
+
+    @Test
+    public void testStopExecutorOnShutdown() throws Exception {


How come this test was moved to the websocket test class?

Probably renamed this class accidentally in IDEA. Will revert this change.

conor10 · 2018-04-15T13:19:16Z

core/src/test/java/org/web3j/protocol/websocket/WebSocketServiceTest.java

+                requestSent.await(2, TimeUnit.SECONDS);
+                sendGethVersionReply();
+            } catch (Exception e) {
+                e.printStackTrace();


Please can this be rethrown instead of discarded.

mushketyk · 2018-04-17T22:10:41Z

Hi @conor10,

Thank you for your review! I am a bit busy at the moment, but I'll try to update the PR next week.

mushketyk · 2018-05-08T07:36:55Z

@conor10 Sorry for the long delay. I've updated the PR and replied to your comments. Could you please review the PR again?

conor10 · 2018-05-10T09:08:53Z

Awesome work 🥇

If you can get those docs written next it would be fantastic.

Websockets support

mushketyk · 2018-10-28T18:19:08Z

@conor10 Sorry, I somehow missed your message. I'll write docs during the next week.

mushketyk · 2018-11-04T17:26:30Z

@conor10 I've added documentation in this PR: #768

mushketyk force-pushed the websockets branch from bbc076b to de04075 Compare March 28, 2018 22:00

conor10 requested changes Apr 15, 2018

View reviewed changes

conor10 added this to the 4.0 milestone Apr 25, 2018

mushketyk added 15 commits May 8, 2018 08:14

Implements WebSocketService

120a951

Implement WebSocket subscriptions

de1395f

Adds Web3jService::close() method

d2133ae

Implements Web3jRx::newHeadsNotifications method

77125f0

Refactor WebSocketService

465376b

Add support for all WebSocket subscriptions

e80c1f3

Adds JavaDoc for WebSocket related classes

313e0c0

Allows a client of WebSocketService to specify unsubscribe method

a10791e

Make WebSocketService thread-safe

c47e923

Close subscription Observable if subscription request failed

754bdb2

Add more unit tests

4152484

Fix unit test race condition

25253a5

Add more methods to WebSocketListener

bfb6491

Close requests and subscriptions when a WebSocket connection is closed

11dffb4

Minor websocket fixes

e574576

mushketyk force-pushed the websockets branch from 80e0afc to e574576 Compare May 8, 2018 07:36

mushketyk mentioned this pull request May 8, 2018

Websockets support in web3j #261

Closed

conor10 merged commit 254afb8 into hyperledger:master May 10, 2018

franz-see pushed a commit to franz-see/web3j that referenced this pull request Aug 3, 2018

Merge pull request hyperledger#458 from mushketyk/websockets

62757fe

Websockets support

snazha-blkio removed this from the 4.0.0-alpha-1 milestone Oct 28, 2018

mushketyk mentioned this pull request Nov 4, 2018

Adds docs for pub/sub related classes and methods #768

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Websockets support #458

Websockets support #458

mushketyk commented Mar 28, 2018 •

edited

Loading

codecov bot commented Mar 28, 2018 •

edited

Loading

mushketyk commented Mar 30, 2018

conor10 commented Apr 1, 2018

mushketyk commented Apr 3, 2018

conor10 left a comment

conor10 Apr 5, 2018

mushketyk May 8, 2018

conor10 Apr 5, 2018

conor10 Apr 15, 2018

mushketyk May 7, 2018

conor10 Apr 15, 2018

mushketyk May 7, 2018

conor10 Apr 15, 2018

mushketyk May 7, 2018

conor10 Apr 15, 2018

mushketyk May 7, 2018

conor10 Apr 15, 2018

mushketyk May 8, 2018

conor10 Apr 15, 2018

mushketyk May 7, 2018

conor10 Apr 15, 2018

mushketyk May 8, 2018

mushketyk May 8, 2018

conor10 Apr 15, 2018

mushketyk May 8, 2018

mushketyk commented Apr 17, 2018

mushketyk commented May 8, 2018

conor10 commented May 10, 2018

mushketyk commented Oct 28, 2018

mushketyk commented Nov 4, 2018


		import rx.subjects.BehaviorSubject;

		public class WebSocketSubscription<T> {

Websockets support #458

Websockets support #458

Conversation

mushketyk commented Mar 28, 2018 • edited Loading

codecov bot commented Mar 28, 2018 • edited Loading

Codecov Report

mushketyk commented Mar 30, 2018

conor10 commented Apr 1, 2018

mushketyk commented Apr 3, 2018

conor10 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

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

mushketyk commented Apr 17, 2018

mushketyk commented May 8, 2018

conor10 commented May 10, 2018

mushketyk commented Oct 28, 2018

mushketyk commented Nov 4, 2018

mushketyk commented Mar 28, 2018 •

edited

Loading

codecov bot commented Mar 28, 2018 •

edited

Loading