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

[network] Confirm and get rid of keep-alives in Notifications protocol #6350

Open
dmitry-markin opened this issue Nov 4, 2024 · 4 comments
Labels
I3-annoyance The node behaves within expectations, however this “expected behaviour” itself is at issue. T0-node This PR/Issue is related to the topic “node”.

Comments

@dmitry-markin
Copy link
Contributor

After #6248 is merged, manually keeping streams alive during additional keep-alive timeout should not be needed.

Confirm this and get rid of it.

@dmitry-markin dmitry-markin added T0-node This PR/Issue is related to the topic “node”. I3-annoyance The node behaves within expectations, however this “expected behaviour” itself is at issue. labels Nov 4, 2024
@nazar-pc
Copy link
Contributor

nazar-pc commented Nov 4, 2024

I believe this is what #6109 does, I specifically did kademlia_legacy_protocol: _, in #6248 to reduce inevitable merge conflict with it.

@dmitry-markin
Copy link
Contributor Author

I believe this is what #6109 does, I specifically did kademlia_legacy_protocol: _, in #6248 to reduce inevitable merge conflict with it.

This seems not related to Notifications protocol keep-alive timeouts for me.

@nazar-pc
Copy link
Contributor

nazar-pc commented Nov 5, 2024

Indeed, I didn't read it carefully 🤦‍♂️

@lexnv
Copy link
Contributor

lexnv commented Nov 13, 2024

Indeed, believe we can remove it. Lets do it as a follow-up to not alter the started tests.

It looks like with this approach we are keeping the connection alive for at least 5 seconds of inactivity:

const INITIAL_KEEPALIVE_TIME: Duration = Duration::from_secs(5);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I3-annoyance The node behaves within expectations, however this “expected behaviour” itself is at issue. T0-node This PR/Issue is related to the topic “node”.
Projects
Status: No status
Development

No branches or pull requests

3 participants