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

feat: add keep alive to Filter #1970

Merged
merged 17 commits into from
Apr 29, 2024
Merged

feat: add keep alive to Filter #1970

merged 17 commits into from
Apr 29, 2024

Conversation

weboko
Copy link
Collaborator

@weboko weboko commented Apr 22, 2024

Problem

Filter doesn't keep keep subscription forever and requires periodic pings.

Solution

Implement keep alive feature to do automatic pings to the peers.

Notes

@weboko weboko requested a review from a team as a code owner April 22, 2024 20:52
@weboko weboko marked this pull request as draft April 22, 2024 20:52
@weboko
Copy link
Collaborator Author

weboko commented Apr 22, 2024

Draft for now before #1958 is merged.

Copy link

github-actions bot commented Apr 22, 2024

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
Waku node 181.09 KB (+0.16% 🔺) 3.7 s (+0.16% 🔺) 1.4 s (-17.96% 🔽) 5 s
Waku Simple Light Node 181.03 KB (-0.09% 🔽) 3.7 s (-0.09% 🔽) 1.7 s (+9.31% 🔺) 5.3 s
ECIES encryption 23.08 KB (0%) 462 ms (0%) 448 ms (+181.83% 🔺) 909 ms
Symmetric encryption 22.55 KB (0%) 452 ms (0%) 330 ms (-25.77% 🔽) 781 ms
DNS discovery 72.42 KB (0%) 1.5 s (0%) 837 ms (+3% 🔺) 2.3 s
Peer Exchange discovery 74.1 KB (0%) 1.5 s (0%) 1.1 s (+4% 🔺) 2.5 s
Local Peer Cache Discovery 67.64 KB (0%) 1.4 s (0%) 657 ms (-31.03% 🔽) 2.1 s
Privacy preserving protocols 38.87 KB (0%) 778 ms (0%) 496 ms (-7.34% 🔽) 1.3 s
Waku Filter 111.76 KB (+0.23% 🔺) 2.3 s (+0.23% 🔺) 1.2 s (+62.87% 🔺) 3.4 s
Waku LightPush 110.11 KB (0%) 2.3 s (0%) 938 ms (-5.49% 🔽) 3.2 s
History retrieval protocols 110.72 KB (0%) 2.3 s (0%) 828 ms (+0.55% 🔺) 3.1 s
Deterministic Message Hashing 7.29 KB (0%) 146 ms (0%) 77 ms (+17.66% 🔺) 223 ms

@weboko weboko marked this pull request as ready for review April 29, 2024 08:25
return;
}

this.keepAliveTimer = window.setInterval(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using window here might be limiting and could cause breaking API in nodejs env

I think just using setInterval/clearInterval should implicitly polyfill

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, the reason why I went with window is to make typescript understand that it is browser API, otherwise I need to suppress types

making it using API directly with type trick

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwise I need to suppress types

the types natively exist in TypeScript -- no need to supress types

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is an ambiguity in types

in nodejs it returns NodeJS.Timeout - https://github.com/DefinitelyTyped/DefinitelyTyped/blob/778ad7a80e9876f258270d84d1db4e6e1d5e3796/types/node/timers.d.ts#L191

where as in browser it is a number

hence the need to do as number to help TS determine it is browser interface

packages/sdk/src/protocols/filter.ts Outdated Show resolved Hide resolved
packages/sdk/src/protocols/filter.ts Show resolved Hide resolved
@adklempner
Copy link
Member

Why can't we use the existing keep alive manager for this? Or why is it insufficient?

@weboko
Copy link
Collaborator Author

weboko commented Apr 29, 2024

Why can't we use the existing keep alive manager for this? Or why is it insufficient?

This is because keepAliveManager does libp2p pings
Overall something like that can be implemented for filter pings once logic becomes more sophisticated: perhaps if next steps are implemented from here #1923 (comment)

@weboko weboko changed the title feat!: add keep alive to Filter feat: add keep alive to Filter Apr 29, 2024
@weboko weboko merged commit 1a6bc4f into master Apr 29, 2024
10 of 11 checks passed
@weboko weboko deleted the weboko/filter-keepalive branch April 29, 2024 21:31
@weboko weboko mentioned this pull request Apr 29, 2024
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.

bug: filter subscription stops without occasional pings
3 participants