Make providers WS and IPC reliable #5763

avkos · 2023-01-17T02:35:55Z

Description

Please include a summary of the changes and be sure to follow our Contribution Guidelines.

Checklist for 4.x:

cloudflare-workers-and-pages · 2023-01-17T02:44:10Z

Deploying with Cloudflare Pages

Latest commit:	`b44fc31`
Status:	✅ Deploy successful!
Preview URL:	https://ac1be8b6.web3-js-docs.pages.dev
Branch Preview URL:	https://ok-5632-make-providers-ws-an.web3-js-docs.pages.dev

View logs

luu-alex · 2023-01-17T18:58:28Z

packages/web3-providers-ipc/package.json

@@ -22,7 +22,7 @@
 		"test:ci": "jest --coverage=true --coverage-reporters=json --verbose",
 		"test:watch": "npm test -- --watch",
 		"test:unit": "jest --config=./test/unit/jest.config.js",
-		"test:integration": "jest --config=./test/integration/jest.config.js --passWithNoTests"
+		"test:integration": "jest --config=./test/integration/jest.config.js --passWithNoTests --forceExit"


Is --forceExit on for debugging?

Locally it is not stuck, but in GitHub Action, sometimes yes. I need to investigate it deeply

If its not fixed in current PR pls create issue pointing to this discussion.

I've fixed the test. So it works without --forceExit

packages/web3-providers-ipc/test/fixtures/helpers.ts

luu-alex · 2023-01-17T19:37:36Z

Looks good! 👍

jdevcs · 2023-01-19T12:27:26Z

packages/web3-providers-ipc/src/index.ts

-	public constructor(socketPath: string) {
-		super(socketPath);
+
+	public constructor(socketPath: string, options?: object, reconnectOptions?: object) {


Is doc updated for this?

ipc provider doesn't have doc

jdevcs · 2023-01-19T12:31:28Z

packages/web3-providers-ipc/src/index.ts

+				});
+			} else {
+				this._connectionStatus = 'disconnected';
+				throw new InvalidConnectionError(this._socketPath);


I think we should also include more error details coming via e in } catch (e) { to throw new InvalidConnectionError(this._socketPath)

jdevcs · 2023-01-19T12:35:59Z

packages/web3-providers-ipc/src/index.ts

-			this._reconnectOptions.autoReconnect &&
-			(![1000, 1001].includes(event.code) || !event.wasClean)
-		) {
+		if (!event && this._reconnectOptions.autoReconnect) {


There can be normal close events are those considered as well ?
so this condition should only be true if there is no clean close or event codes are not 1000 and 1001,

IPC providers use import { Socket } from 'net'; to connect.
in the close event of Socket we do not have any code or wasClean information.
That is why for IPC we have a different condition.

but when we call the disconnect method we create an event on our own. and in this case, we will have an event object here.

ok, so is this tested for normal close, its existing without reconnect?

yes, this is a test "exit without reconnection":

it('should emit connect and disconnected events', async () => { const server = await startGethServer(8547); const web3Provider = new IpcProvider(server.path); expect(!!(await waitForEvent(web3Provider, 'connect'))).toBe(true); const disconnectPromise = waitForEvent(web3Provider, 'disconnect'); web3Provider.disconnect(); expect(!!(await disconnectPromise)).toBe(true); await server.close(); });

jdevcs · 2023-01-19T12:37:29Z

packages/web3-providers-ws/src/index.ts

@@ -76,13 +72,16 @@ export default class WebSocketProvider<

 			this._addSocketListeners();
 		} catch (e) {
-			throw new InvalidConnectionError(this._socketPath);
+			if (!this.isReconnecting) {
+				throw new InvalidConnectionError(this._socketPath);


pls also include more error details from e obj in rethrow

jdevcs · 2023-01-24T12:08:49Z

LGTM, Some of WS tests are not invoked in CI, pls check before merging if there is any thing with current PR causing it. @avkos

Muhammad-Altabba

Looks great!
However, I have a few comments.

packages/web3-providers-ipc/test/fixtures/helpers.ts

Muhammad-Altabba · 2023-01-25T09:55:39Z

packages/web3-providers-ws/test/integration/reconnection.test.ts

+		it('check defaults', async () => {
+			const web3Provider = new WebSocketProvider(getSystemTestProvider());
+			// @ts-expect-error-next-line
+			expect(web3Provider._reconnectOptions).toEqual({
+				autoReconnect: true,
+				delay: 5000,
+				maxAttempts: 5,
+			});
+			await waitForOpenConnection(web3Provider);
+			web3Provider.disconnect(1000, 'test');
+			await waitForCloseConnection(web3Provider);
+		});
+		it('set custom reconnectOptions', async () => {
+			const web3Provider = new WebSocketProvider(
+				getSystemTestProvider(),
+				{},
+				reconnectionOptions,
+			);
+			// @ts-expect-error-next-line
+			expect(web3Provider._reconnectOptions).toEqual(reconnectionOptions);
+			await waitForOpenConnection(web3Provider);
+			web3Provider.disconnect(1000, 'test');
+			await waitForCloseConnection(web3Provider);
+		});


Maybe the logic of those 2 tests could be moved to web3-utils. We just need to keep the Provider initialization. And the rest is douplicate with IpcProvider. Or what do you think?

we can have tests here and in web3-utils just to be sure it works not only on base class but in WebsocketProvider and in IPCProvider

Yes, I meant to have only the provider initialization at this package:

const web3Provider = new WebSocketProvider( getSystemTestProvider(), {}, reconnectionOptions, );

And then the object web3Provider could be passed to a function that contains the logic (that is exactly the same for both WS and IPC):

// @ts-expect-error-next-line expect(web3Provider._reconnectOptions).toEqual(reconnectionOptions); await waitForOpenConnection(web3Provider); web3Provider.disconnect(1000, 'test'); await waitForCloseConnection(web3Provider);

scripts/geth.sh

Muhammad-Altabba · 2023-01-25T10:04:21Z

scripts/geth.sh

 	if [ -z "${ORIGARGS[1]}" ]
 	then
 		echo "Starting geth..."
-		echo "docker run -p $WEB3_SYSTEM_TEST_PORT:$WEB3_SYSTEM_TEST_PORT --name web3-geth-client ethereum/client-go --nodiscover --nousb --ws --ws.addr 0.0.0.0 --ws.port $WEB3_SYSTEM_TEST_PORT --http --http.addr 0.0.0.0 --http.port $WEB3_SYSTEM_TEST_PORT --allow-insecure-unlock --http.api personal,web3,eth,admin,debug,txpool,net --ws.api personal,web3,eth,admin,debug,miner,txpool,net --dev"
-        docker run -p $WEB3_SYSTEM_TEST_PORT:$WEB3_SYSTEM_TEST_PORT ethereum/client-go --nodiscover --nousb --ws --ws.addr 0.0.0.0 --ws.port $WEB3_SYSTEM_TEST_PORT --http --http.addr 0.0.0.0 --http.port $WEB3_SYSTEM_TEST_PORT --allow-insecure-unlock --http.api personal,web3,eth,admin,debug,txpool,net --ws.api personal,web3,eth,admin,debug,miner,txpool,net --dev
+		echo "docker run -p $WEB3_SYSTEM_TEST_PORT:$WEB3_SYSTEM_TEST_PORT ethereum/client-go:stable --nodiscover --nousb --ws --ws.addr 0.0.0.0 --ws.port $WEB3_SYSTEM_TEST_PORT --http --http.addr 0.0.0.0 --http.port $WEB3_SYSTEM_TEST_PORT --allow-insecure-unlock --http.api personal,web3,eth,admin,debug,txpool,net --ws.api personal,web3,eth,admin,debug,miner,txpool,net --dev"


I think having a name for the docker container is something good. I mean to keep --name web3-geth-client

let's make changes in docker in another PR

Yes, for this I recommend not changing the docker configuration related to the docker image name (--name web3-geth-client) in this MR 😄

Muhammad-Altabba

Great work Oleksii,
I have just 2 minor comments that you may consider. And you may merge without considering them.

ws and ipc reconnection

d6011c1

github-actions bot temporarily deployed to Preview: (ok/5632-Make-providers-WS-and-IPC-reliable) January 17, 2023 02:43 Inactive

fix

1a96ba2

github-actions bot temporarily deployed to Preview: (ok/5632-Make-providers-WS-and-IPC-reliable) January 17, 2023 02:52 Inactive

increase time for IPC

dcada50

github-actions bot temporarily deployed to Preview: (ok/5632-Make-providers-WS-and-IPC-reliable) January 17, 2023 03:06 Inactive

fix ipc

22b8b34

github-actions bot temporarily deployed to Preview: (ok/5632-Make-providers-WS-and-IPC-reliable) January 17, 2023 03:25 Inactive

luu-alex reviewed Jan 17, 2023

View reviewed changes

packages/web3-providers-ipc/test/fixtures/helpers.ts Outdated Show resolved Hide resolved

jdevcs reviewed Jan 19, 2023

View reviewed changes

jdevcs requested changes Jan 19, 2023

View reviewed changes

jdevcs reviewed Jan 19, 2023

View reviewed changes

add error messages. add ganache to dev dep

96cf9da

github-actions bot temporarily deployed to Preview: (ok/5632-Make-providers-WS-and-IPC-reliable) January 19, 2023 22:21 Inactive

add ganache install to actions

43c1d8c

github-actions bot temporarily deployed to Preview: (ok/5632-Make-providers-WS-and-IPC-reliable) January 19, 2023 22:48 Inactive

ws reconnection test with geth

6758a8b

github-actions bot temporarily deployed to Preview: (ok/5632-Make-providers-WS-and-IPC-reliable) January 19, 2023 23:51 Inactive

run as a docker

0a674f6

github-actions bot temporarily deployed to Preview: (ok/5632-Make-providers-WS-and-IPC-reliable) January 20, 2023 02:06 Inactive

Merge branch '4.x' into ok/5632-Make-providers-WS-and-IPC-reliable

37304cf

github-actions bot temporarily deployed to Preview: (ok/5632-Make-providers-WS-and-IPC-reliable) January 20, 2023 10:41 Inactive

debug.trace

62440f7

github-actions bot temporarily deployed to Preview: (ok/5632-Make-providers-WS-and-IPC-reliable) January 20, 2023 22:35 Inactive

avkos added 2 commits January 20, 2023 17:58

test

d4e6808

skip tests

8bd002a

avkos requested review from Muhammad-Altabba and luu-alex January 24, 2023 00:02

github-actions bot temporarily deployed to Preview: (ok/5632-Make-providers-WS-and-IPC-reliable) January 24, 2023 00:09 Inactive

fix unit test

46fb90b

github-actions bot temporarily deployed to Preview: (ok/5632-Make-providers-WS-and-IPC-reliable) January 24, 2023 00:33 Inactive

jdevcs approved these changes Jan 24, 2023

View reviewed changes

luu-alex approved these changes Jan 24, 2023

View reviewed changes

Merge branch '4.x' into ok/5632-Make-providers-WS-and-IPC-reliable

3b1953c

github-actions bot temporarily deployed to Preview: (ok/5632-Make-providers-WS-and-IPC-reliable) January 24, 2023 14:56 Inactive

try to stop docker

61f6616

github-actions bot temporarily deployed to Preview: (ok/5632-Make-providers-WS-and-IPC-reliable) January 24, 2023 15:23 Inactive

wait port timeout

f22cac3

github-actions bot temporarily deployed to Preview: (ok/5632-Make-providers-WS-and-IPC-reliable) January 24, 2023 15:33 Inactive

kill 8547 port

48d099a

github-actions bot temporarily deployed to Preview: (ok/5632-Make-providers-WS-and-IPC-reliable) January 24, 2023 15:46 Inactive

try stable version of geth

9f6a065

github-actions bot temporarily deployed to Preview: (ok/5632-Make-providers-WS-and-IPC-reliable) January 24, 2023 16:03 Inactive

fix

69f6e30

github-actions bot temporarily deployed to Preview: (ok/5632-Make-providers-WS-and-IPC-reliable) January 24, 2023 16:47 Inactive

fix path

6247646

github-actions bot temporarily deployed to Preview: (ok/5632-Make-providers-WS-and-IPC-reliable) January 24, 2023 18:08 Inactive

nikoulai approved these changes Jan 25, 2023

View reviewed changes

Muhammad-Altabba reviewed Jan 25, 2023

View reviewed changes

move helpers

b44fc31

github-actions bot deployed to Preview: (ok/5632-Make-providers-WS-and-IPC-reliable) January 25, 2023 16:07 View deployment

avkos requested a review from Muhammad-Altabba January 25, 2023 16:26

Muhammad-Altabba approved these changes Jan 25, 2023

View reviewed changes

avkos merged commit 8e44e88 into 4.x Jan 25, 2023

avkos deleted the ok/5632-Make-providers-WS-and-IPC-reliable branch January 25, 2023 17:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make providers WS and IPC reliable #5763

Make providers WS and IPC reliable #5763

avkos commented Jan 17, 2023

cloudflare-workers-and-pages bot commented Jan 17, 2023 •

edited

Loading

luu-alex Jan 17, 2023

avkos Jan 17, 2023

jdevcs Jan 19, 2023

avkos Jan 24, 2023

luu-alex commented Jan 17, 2023

jdevcs Jan 19, 2023

avkos Jan 19, 2023

jdevcs Jan 19, 2023

avkos Jan 19, 2023

jdevcs Jan 19, 2023

avkos Jan 19, 2023

jdevcs Jan 20, 2023

avkos Jan 20, 2023

jdevcs Jan 24, 2023

jdevcs Jan 19, 2023

avkos Jan 19, 2023

jdevcs commented Jan 24, 2023

Muhammad-Altabba left a comment

Muhammad-Altabba Jan 25, 2023

avkos Jan 25, 2023

Muhammad-Altabba Jan 25, 2023

Muhammad-Altabba Jan 25, 2023

avkos Jan 25, 2023

Muhammad-Altabba Jan 25, 2023 •

edited

Loading

Muhammad-Altabba left a comment

Make providers WS and IPC reliable #5763

Make providers WS and IPC reliable #5763

Conversation

avkos commented Jan 17, 2023

Description

Checklist for 4.x:

cloudflare-workers-and-pages bot commented Jan 17, 2023 • edited Loading

Deploying with Cloudflare Pages

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

luu-alex commented Jan 17, 2023

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

jdevcs commented Jan 24, 2023

Muhammad-Altabba 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

Muhammad-Altabba Jan 25, 2023 • edited Loading

Choose a reason for hiding this comment

Muhammad-Altabba left a comment

Choose a reason for hiding this comment

cloudflare-workers-and-pages bot commented Jan 17, 2023 •

edited

Loading

Muhammad-Altabba Jan 25, 2023 •

edited

Loading