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 custom events to Relay #1213

Merged
merged 9 commits into from
Mar 15, 2023
Merged

feat: add custom events to Relay #1213

merged 9 commits into from
Mar 15, 2023

Conversation

weboko
Copy link
Collaborator

@weboko weboko commented Mar 1, 2023

Problem

There was a report that Relay.observers were not properly cleaned up and it was hard to debug in a runtime so some hack was introduced on the consumer side.

Solution

Add observer:added and observer:removed to track state of the internal observers structure.

Notes

Validation

image

Copy link
Collaborator

@fryorcraken fryorcraken left a comment

Choose a reason for hiding this comment

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

Looks good. I only wonder if we should use CustomEvents from libp2p or have our own. The event does not seem libp2p related so just wonder if we are restricting ourselves here.

@weboko
Copy link
Collaborator Author

weboko commented Mar 2, 2023

Looks good. I only wonder if we should use CustomEvents from libp2p or have our own. The event does not seem libp2p related so just wonder if we are restricting ourselves here.

Not really, CustomEvent is just a browser abstract and we can avoid using it altogether any time even now.
Used it just to be consistent with GossipSub

@weboko weboko marked this pull request as ready for review March 13, 2023 23:33
@weboko weboko requested a review from fryorcraken March 13, 2023 23:33
Copy link
Collaborator

@fryorcraken fryorcraken left a comment

Choose a reason for hiding this comment

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

Looks good bug I wonder if it's good enough for @therealjmj. We might need a dump function too.

packages/core/src/lib/relay/index.ts Outdated Show resolved Hide resolved
packages/core/src/lib/relay/index.ts Show resolved Hide resolved
@weboko weboko mentioned this pull request Mar 14, 2023
3 tasks
@github-actions
Copy link

github-actions bot commented Mar 14, 2023

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
Waku core 116.74 KB (+0.03% 🔺) 2.4 s (+0.03% 🔺) 992 ms (-11.49% 🔽) 3.4 s
Waku default setup 384.04 KB (+0.02% 🔺) 7.7 s (+0.02% 🔺) 2.3 s (-6.69% 🔽) 10 s
ECIES encryption 27.99 KB (0%) 560 ms (0%) 387 ms (-22.48% 🔽) 947 ms
Symmetric encryption 27.99 KB (0%) 560 ms (0%) 501 ms (+12.07% 🔺) 1.1 s
DNS discovery 108.07 KB (0%) 2.2 s (0%) 1.2 s (+13.64% 🔺) 3.3 s
Privacy preserving protocols 116.22 KB (+0.03% 🔺) 2.4 s (+0.03% 🔺) 922 ms (-9.74% 🔽) 3.3 s
Light protocols 117.98 KB (+0.03% 🔺) 2.4 s (+0.03% 🔺) 961 ms (+14.95% 🔺) 3.4 s
History retrieval protocols 118.06 KB (+0.03% 🔺) 2.4 s (+0.03% 🔺) 746 ms (-7.84% 🔽) 3.2 s

@weboko weboko merged commit 275b166 into master Mar 15, 2023
@weboko weboko deleted the weboko/observers-event branch March 15, 2023 20:47
@weboko weboko restored the weboko/observers-event branch March 15, 2023 20:48
@weboko weboko deleted the weboko/observers-event branch March 15, 2023 20:48
@therealjmj
Copy link

Looks good bug I wonder if it's good enough for @therealjmj. We might need a dump function too.

Funny, in making 'observers' private, it's likely this broke our implementation.
We just want a way to log the current observers, as you say, a "dump".

@weboko
Copy link
Collaborator Author

weboko commented Mar 16, 2023

@therealjmj I can followup with a "dump" function but isn't it enough to have these two events for you to track what happens to observers? If you have a need for full observers list it can be maintained by these events too.

@therealjmj
Copy link

therealjmj commented Mar 16, 2023

@therealjmj I can followup with a "dump" function but isn't it enough to have these two events for you to track what happens to observers? If you have a need for full observers list it can be maintained by these events too.

I'm really not a fan of events and listener/subscriber models - it'll add tech that needs to be tested and maintained on our end.

However... I would love a "dump" function to make it much easier on us!

@weboko
Copy link
Collaborator Author

weboko commented Mar 16, 2023

Ok :) Adding dump function soon

upd: added here #1249

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.

Relay interface doesn't include 'observers'
3 participants