Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add config.keepaliveForce #419

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

gerardo15
Copy link

@gerardo15 gerardo15 commented Jul 4, 2021

Why?

If you have ever worked with web3 subscriptions, in particular with infura's node, you will stop receiving messages after a few minutes of inactivity.

When a subscription its established for example a subscription to logs, there would be only incomming messages from the node.

The package currently doesn't ping if messages are being received, so after a few minutes infura will close the connection because it has not received any ping (keepalive)

I propose adding this option keepaliveForce, that would force the keepalive to be sent

Here you can see workarounds that need to be done, but could be solved if this option its enabled

https://medium.com/@pauloostenrijk/infura-drops-your-websocket-connections-after-a-while-and-here-is-a-fix-for-it-3413ca8253b

Why?

If you have ever worked with web3 subscriptions, in particular with infura's node, you will stop receiving messages after a few minutes of inactivity.

When a subscription its established for example a subscription to logs, there would be only incomming messages from the node.

The package currently doesn't ping if messages are being received.

I propose adding this option keepaliveForce, that would force the keepalive to be sent

Here you can see workarounds that need to be done, but could be solved if this option its enabled

https://medium.com/@pauloostenrijk/infura-drops-your-websocket-connections-after-a-while-and-here-is-a-fix-for-it-3413ca8253b
@ibc
Copy link
Collaborator

ibc commented Jul 8, 2021

I fail to understand what changes in this PR fix the problem you mention in the PR description. So:

  1. Can you please detail it a bit more?
  2. Such a new option must be documented in the docs folder.

@gerardo15
Copy link
Author

gerardo15 commented Jul 8, 2021

I fail to understand what changes in this PR fix the problem you mention in the PR description. So:

  1. Can you please detail it a bit more?
  2. Such a new option must be documented in the docs folder.

Hey! Let me explain a lit better.

  1. Keep in mind this problem happens with a Client connection to a server, and this servers expects pings every 60 seconds (or any other interval) to not close the conneciton, if the client does not send a ping, the server will close the connection, Current implementation only sends ping (keepalive) if there is no data being recieved on the client, so in the example above when you are subscribed to messages and not sending any messages, you as the client will have incomming messages and the keepalive timer will always reset, if the server expects a ping / keepalive to not close the connection no pings will be sent to the server, so connection would be closed. if you set keepaliveForce to true, it will force ping to be sent to the server even if new messages are incomming so the server will not close the connection.

  2. I have updated the docs.

@ibc
Copy link
Collaborator

ibc commented Jul 12, 2021

I understand now, thanks. I'll approve this PR but I'm not the maintainer and cannot make a release.
@theturtle32 any change to merge and release?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants