2. Refactor Connection & Retry Handling #160

timoxley · 2020-09-08T18:57:48Z

Moved majority of connection & retry handling logic into Connection.
Connection is checked and potentially reconnected whenever socket is written to, simplifying everything that writes to socket.
Improved reconnection/autoconnect logic, should be more predictable now.
Manually calling disconnect() disables autoconnect. Will only reconnect if asked.
Cleaned up and improved logging significantly, e.g. now logs the request object/deserialised response rather than hard to read serialized array.
Error handling also improved, tried to prevent cases of double logging or even double emitting the same error.
connect/disconnect now roughly equivalent what ensureConnected/ensureDisconnected did, methods left for backwards compatibility, could be removed though.

Builds on:

Split Client into Publisher/Subscriber/Resender 1. Remove Encryption, Refactor Client #156

Followed by:

Publish/Subscribe Promisification 3. Promisify subscribe/unsubscribe methods #162
Publish Refactoring 4. Refactor Publish #164

…stent with bad url parsing errors.

…SAGE_TYPES to CONTENT_TYPES.

…test

…rrors consistent.

mirpo

lgtm

mirpo · 2020-09-21T06:38:10Z

src/Connection.js

@@ -1,169 +1,614 @@
 import EventEmitter from 'eventemitter3'
-import debugFactory from 'debug'
+import Debug from 'debug'


how about pino later?

yep pino looks good to me, that's on my todo

mirpo · 2020-09-21T06:39:15Z

src/Connection.js

+                throw new ConnectionError('URL is not defined!')
+            }
+            const socket = new WebSocket(url, ...args)
+            socket.id = uniqueId('socket')


@harbu loves => monkey patching

mirpo · 2020-09-21T06:40:51Z

src/Connection.js

+            let opened = 0
+            socket.onopen = () => {
+                opened = 1
+                openSockets += opened


for what do you need openSockets and opened = 1?

openSockets -= opened

the opened is for bookkeeping, onclose should only decrement if we actually incremented the counter. Could be a bool I guess but 🤷

I keep track of openSockets for tests, exposed via Connection.getOpen otherwise it's easy to end up opening two connections and only closing one. Yep, this code exists for tests only which is a no-no, but it's just a counter and provides high signal vs noise i.e. should never have any open sockets at end of a test.

afterEach(async () => { await client.disconnect() const openSockets = Connection.getOpen() if (openSockets !== 0) { throw new Error(`sockets not closed: ${openSockets}`) } })

As this is only for tests, it shouldn't be a big problem.

We ended up with a similar sort of counter not being an accurate representation of the actual number of live connections in a broker node. Seemed like the disconnect event didn't always trigger. Apparently if the connection got timed out or smth, the disconnect event wouldn't trigger. Ended up using a counter within the library to print the connection count to logs.

mirpo · 2020-09-21T06:42:59Z

src/Connection.js

+        }
+
+        if (socket.readyState === WebSocket.CLOSING) {
+            socket.addEventListener('close', resolve)


I remember in the network when socket stuck in CONNECTING state. Maybe it's better to add case for it?

Was that using ws directly or using the old client? socket.readyState === WebSocket.OPENING is the connecting state, which is checked above, but does only wait for 'error'/'open' events, assumes these are the only two possible events. Perhaps it should also wait for 'close' in the case of a close without an error before it hits 'open'? and perhaps could do with a timeout. Any idea how to trigger that stuck state?

mirpo · 2020-09-21T06:45:16Z

src/Connection.js

-            })
+        // note if event handler is async and it rejects we're kinda hosed
+        // until node lands unhandledrejection support
+        // in eventemitter


We are using import EventEmitter from 'eventemitter3' is this correct?

I'm not sure if that will ever be updated to capture rejections.

Check this: https://nodejs.org/api/events.html#events_capture_rejections_of_promises

https://github.com/nodejs/node/blob/17ebd464ccdf12a4fb46334ff5d7a71f0f2e70a9/lib/events.js#L165-L206

harbu · 2020-09-22T09:18:22Z

src/Connection.js

+                maxRetryWait, // max wait time
+                Math.round((this.retryCount * 10) ** retryBackoffFactor)
+            )
+            if (!timeout) {


So only log when timeout === 0? When would this occur?

harbu · 2020-09-22T09:20:28Z

src/Connection.js

+    async backoffWait() {
+        const { retryBackoffFactor = 1.2, maxRetryWait = 10000 } = this.options
+        return new Promise((resolve) => {
+            clearTimeout(this._backoffTimeout)


What happens to those that await the previous (old) promise? Should we reject those?

harbu · 2020-09-22T09:26:39Z

src/Connection.js

+        try {
+            result = super.emit(event, ...args)
+        } catch (err) {
+            super.emit('error', err)


Wonder if the error that occurs in a listener should be considered an error of the connection?

harbu · 2020-09-22T09:29:52Z

src/Connection.js

-        this.socket.binaryType = 'arraybuffer'
-        this.socket.events = new EventEmitter()
+        const reconnectTask = (async () => {
+            if (this.retryCount > maxRetries) {


So does the client stop trying to re-connect after maxRetries?

harbu · 2020-09-22T11:53:48Z

src/Connection.js

+                }
+
+                this.debug = this._debug
+                if (this.initialConnectTask === initialConnectTask) {


Same as above in function?

harbu · 2020-09-22T11:54:34Z

src/Connection.js

+        return this.initialConnectTask
+    }
+
+    async _connectOnce() {


Wonder if connect / reconnect logic could be extracted to be just one function?

* Promisify subscribe/unsubscribe. * Fail _request call if unsubscribe/subscribe request fails to send. * Fix missing import in resend test. * Fix browser tests. * Clean up unused function in test. * 4. Refactor Publish (#164) * Refactor & co-locate publish code. * Avoid using expensive ethers.Wallet.createRandom() calls in test. e.g. 1000x calls with ethers: 14s, randomBytes: 3.5ms. * Ensure messageCreationUtil is cleaned up after test. * Fix non-functional MessageCreationUtil test. * Swap out receptacle for more flexible mem/p-memoize/quick-lru. * Convert LoginEndpoints test to async/await. * Remove calls to ensureConnected/ensureDisconnected in test. * 5. Message Sequencing – Guarantee sequence follows publish order & Prevent backdated messages silently breaking future publishes (#166) * Improve authFetch logging. * Update message sequencer to strictly enforce message order. * Queue publishes per-stream otherwise can skip forward then back even when messages published in correct sequence. * Add partial solution to broken backdated messages, at least doesn't break regular sequential publishes. * Tidy up, add some comments. * Move publish queue fn into utils.

timoxley force-pushed the refactor-connection branch from 59d4991 to fa2487a Compare September 8, 2020 19:10

timoxley changed the title ~~Refactor connection (WIP)~~ Refactor Connection & Retry Handling Sep 14, 2020

timoxley force-pushed the refactor-connection branch 2 times, most recently from 6c7bc5f to 088d24f Compare September 14, 2020 19:13

timoxley changed the base branch from master to remove-encryption-code September 14, 2020 19:22

timoxley added 25 commits September 15, 2020 10:33

Disable max-classes-per-file eslint rule.

876d2fa

Add SocketConnection class.

fe5ccef

Improve reopening & failure handling.

ea7e3a9

Split reopening logic into SocketConnection subclass.

2943ec7

Refactor open/close logic, move no url error to open call to be consi…

d679bd0

…stent with bad url parsing errors.

Convert event handler exceptions to error events. Tidy code coverage.

c2637c8

Simplify SocketConnection.

1d62e4e

Tidy up Connection, s/open/connect.

41faba6

Tune reconnection, finish s/open/connect.

ce60a54

Move autoconnect logic into connection.

369fc69

Isolate open/close functions. Tighten reconnect.

e71c566

Integrate SocketConnection API, pass unit/StreamrClient.test.js.

0daabdd

Remove old Connection.

6379344

Test connect/disconnect in event handlers.

1483c00

Add tests around nextConnection.

79d2d47

Disable autoConnect if manually disconnected.

a24c253

Local tests passing for refactored connection integration.

b74f4c7

Lint, tidy up.

e0b6523

Update messageType to contentType & more references from protocol MES…

f44c0cd

…SAGE_TYPES to CONTENT_TYPES.

Increase Connection test timeout.

fd9bc32

Fix browser tests.

5902432

Move Connection into src.

2bfd3f8

Tidy up debug logging.

6c8220a

Remove unused from Connection.

99fea89

Tune subscriber logging.

ddabc0b

timoxley force-pushed the refactor-connection branch from 088d24f to ddabc0b Compare September 15, 2020 15:12

timoxley added 7 commits September 15, 2020 12:44

Fix default retry options when options object gets rewritten e.g. in …

2460110

…test

Fail resend request if error sending.

c978f5f

Adjust connection error handling to be more consistent about emit/throw.

9105acb

Remove client.handleError, make emit/throw & ignoring of Connection e…

d6f829a

…rrors consistent.

Don't throw, only emit errors for unchained async tasks.

2009c5b

Disable logging in long resend test.

dc49a36

Clean up unused imports.

abea2d3

timoxley force-pushed the refactor-connection branch from 236eb73 to abea2d3 Compare September 15, 2020 18:43

timoxley mentioned this pull request Sep 17, 2020

1. Remove Encryption, Refactor Client #156

Closed

timoxley requested review from harbu, hpihkala and mirpo September 17, 2020 13:36

timoxley mentioned this pull request Sep 17, 2020

4. Refactor Publish #164

Merged

Install streamr-client-protocol@5.1.0

49921e8

mirpo approved these changes Sep 21, 2020

View reviewed changes

timoxley changed the title ~~Refactor Connection & Retry Handling~~ 2. Refactor Connection & Retry Handling Sep 21, 2020

timoxley mentioned this pull request Sep 21, 2020

5. Message Sequencing – Guarantee sequence follows publish order & Prevent backdated messages silently breaking future publishes #166

Merged

harbu approved these changes Sep 22, 2020

View reviewed changes

timoxley mentioned this pull request Nov 24, 2020

Streamr Client 5.0.0 #167

Closed

timoxley merged commit 4c4e534 into remove-encryption-code Jan 19, 2021

timoxley deleted the refactor-connection branch January 19, 2021 18:34

timoxley mentioned this pull request Jan 20, 2021

Streamr Client 5.0.0 #192

Merged

2. Refactor Connection & Retry Handling #160

2. Refactor Connection & Retry Handling #160

Uh oh!

Conversation

timoxley commented Sep 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mirpo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timoxley Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

timoxley commented Sep 8, 2020 •

edited

Loading

timoxley Sep 21, 2020 •

edited

Loading