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.

Sign up for GitHub

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

Jump to bottom

Allow multiple subs for same (peer, contentFilter) pairs #762

Merged
merged 1 commit into from
Sep 22, 2023
Merged

Allow multiple subs for same (peer, contentFilter) pairs #762

merged 1 commit into from
Sep 22, 2023

Conversation

vitvly
Copy link
Contributor

@vitvly vitvly commented Sep 21, 2023 •
edited
Loading

Following changes are included:

  • allow creating a new Subscription for same (peer, contentFilter) pairs (this was mistakenly assumed to be wrong and thus removed in feat: topic refactors for filter client #750)
  • modify UnsubscribeWithSubscription so that it may close a single subscription only, and not all with a given contentFilter
  • merge FilterSubscribe and FilterUnsubscribe options/params, there was some overlap there
  • resource cleanup: close subscription channels when unsubscribing
  • minor refactors
  • test updates

@vitvly vitvly self-assigned this Sep 21, 2023
@status-im-auto
Copy link

status-im-auto commented Sep 21, 2023 •
edited
Loading

Jenkins Builds

Click to see older builds (44)
❔ Commit #️⃣ Finished (UTC) Duration Platform Result
✖️ 897a4d8 #1 2023-09-21 10:50:16 ~1 min tests 📄log
✖️ 897a4d8 #1 2023-09-21 10:50:16 ~1 min tests 📄log
✖️ 897a4d8 #1 2023-09-21 10:50:46 ~2 min nix-flake 📄log
❌ 897a4d8 #1 2023-09-21 10:50:52 ~2 min linux 📄log
❌ 897a4d8 #1 2023-09-21 10:51:46 ~3 min android 📄log
❌ 897a4d8 #1 2023-09-21 10:51:51 ~3 min ios 📄log
✖️ 7076dd3 #2 2023-09-21 10:55:04 ~23 sec tests 📄log
✔️ 7076dd3 #2 2023-09-21 10:55:49 ~1 min linux 📦deb
✖️ 7076dd3 #2 2023-09-21 10:56:03 ~1 min tests 📄log
✔️ 7076dd3 #2 2023-09-21 10:56:47 ~2 min nix-flake 📄log
✔️ 7076dd3 #2 2023-09-21 10:58:10 ~3 min ios 📦tgz
✔️ 7076dd3 #2 2023-09-21 10:58:36 ~4 min android 📦tgz
✔️ 488fc55 #3 2023-09-21 11:05:06 ~1 min linux 📦deb
✖️ 488fc55 #3 2023-09-21 11:05:41 ~1 min tests 📄log
✔️ 488fc55 #3 2023-09-21 11:06:00 ~2 min nix-flake 📄log
✔️ 488fc55 #3 2023-09-21 11:06:07 ~2 min tests 📄log
✔️ 488fc55 #3 2023-09-21 11:07:15 ~3 min ios 📦tgz
✔️ 488fc55 #3 2023-09-21 11:08:12 ~4 min android 📦tgz
✔️ 035dc75 #4 2023-09-21 11:12:19 ~1 min nix-flake 📄log
✔️ 035dc75 #4 2023-09-21 11:12:41 ~2 min linux 📦deb
✔️ 035dc75 #4 2023-09-21 11:13:30 ~3 min tests 📄log
✔️ 035dc75 #4 2023-09-21 11:13:38 ~3 min tests 📄log
✔️ 035dc75 #4 2023-09-21 11:13:39 ~3 min ios 📦tgz
✔️ 035dc75 #4 2023-09-21 11:13:59 ~3 min android 📦tgz
✔️ 7d77b67 #5 2023-09-22 11:01:12 ~1 min linux 📦deb
✔️ 7d77b67 #5 2023-09-22 11:01:50 ~1 min tests 📄log
✔️ 7d77b67 #5 2023-09-22 11:01:57 ~1 min nix-flake 📄log
✔️ 7d77b67 #5 2023-09-22 11:02:11 ~2 min tests 📄log
✔️ 7d77b67 #5 2023-09-22 11:03:40 ~3 min android 📦tgz
✔️ 7d77b67 #5 2023-09-22 11:04:22 ~4 min ios 📦tgz
✔️ 64f3da6 #6 2023-09-22 13:51:13 ~1 min nix-flake 📄log
✔️ 64f3da6 #6 2023-09-22 13:51:27 ~2 min tests 📄log
✔️ 64f3da6 #6 2023-09-22 13:51:39 ~2 min linux 📦deb
✖️ 64f3da6 #6 2023-09-22 13:51:57 ~2 min tests 📄log
✔️ 64f3da6 #6 2023-09-22 13:52:37 ~3 min android 📦tgz
✔️ 64f3da6 #6 2023-09-22 13:52:48 ~3 min ios 📦tgz
✖️ 64f3da6 #7 2023-09-22 14:09:57 ~1 min tests 📄log
✖️ 64f3da6 #8 2023-09-22 14:12:29 ~1 min tests 📄log
✔️ d6fed8e #7 2023-09-22 14:22:11 ~1 min linux 📦deb
✖️ d6fed8e #7 2023-09-22 14:22:26 ~1 min tests 📄log
✔️ d6fed8e #9 2023-09-22 14:22:50 ~1 min tests 📄log
✔️ d6fed8e #7 2023-09-22 14:23:05 ~1 min nix-flake 📄log
✔️ d6fed8e #7 2023-09-22 14:24:43 ~3 min android 📦tgz
✔️ d6fed8e #7 2023-09-22 14:25:06 ~3 min ios 📦tgz
❔ Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 391b05b #8 2023-09-22 14:46:36 ~58 sec linux 📦deb
✔️ 391b05b #10 2023-09-22 14:47:19 ~1 min tests 📄log
✔️ 391b05b #8 2023-09-22 14:47:34 ~1 min nix-flake 📄log
✔️ 391b05b #8 2023-09-22 14:47:39 ~1 min tests 📄log
✔️ 391b05b #8 2023-09-22 14:48:44 ~3 min android 📦tgz
✔️ 391b05b #8 2023-09-22 14:49:02 ~3 min ios 📦tgz
✔️ 915f77f #11 2023-09-22 14:49:46 ~49 sec tests 📄log
✔️ 915f77f #9 2023-09-22 14:49:58 ~1 min linux 📦deb
✔️ 915f77f #9 2023-09-22 14:50:50 ~1 min nix-flake 📄log
✔️ 915f77f #9 2023-09-22 14:50:56 ~1 min tests 📄log
✔️ 915f77f #9 2023-09-22 14:52:02 ~3 min android 📦tgz
✔️ 915f77f #9 2023-09-22 14:52:48 ~3 min ios 📦tgz

@vitvly vitvly mentioned this pull request Sep 21, 2023
Closed
@vitvly vitvly force-pushed the refactor/filter-unsubscribe branch from 897a4d8 to 7076dd3 Compare September 21, 2023 10:54
@vitvly vitvly force-pushed the refactor/filter-unsubscribe branch 2 times, most recently from 488fc55 to 035dc75 Compare September 21, 2023 11:10
@chaitanyaprem
Copy link
Collaborator

allow creating a new Subscription for same (peer, contentFilter) pairs (this was mistakenly assumed to be wrong and thus removed in #750)

Out of curiosity, unable to understand a use-case where we would want something like this. Wouldn't this cause stale older subscriptions to remain and references lost to them in the filterClient?

@vitvly
Copy link
Contributor Author

vitvly commented Sep 22, 2023 •
edited
Loading

Testing this in status-im/status-go#4020
gives

panic: runtime error: slice bounds out of range [2:1]
goroutine 1232 [running]:
github.com/waku-org/go-waku/waku/v2/peerstore.(*WakuPeerstoreImpl).RemovePubSubTopic(0xc00117d290, {0xc001459560, 0x27}, {0x0?, 0x1a?})
	/home/jenkins/workspace/status-go_prs_tests_PR-4020/vendor/github.com/waku-org/go-waku/waku/v2/peerstore/waku_peer_store.go:174 +0x1f0
github.com/waku-org/go-waku/waku/v2/peermanager.(*PeerManager).peerEventLoop(0xc000716a20, {0x1669418, 0xc000541ac0})
	/home/jenkins/workspace/status-go_prs_tests_PR-4020/vendor/github.com/waku-org/go-waku/waku/v2/peermanager/peer_manager.go:109 +0x25c
created by github.com/waku-org/go-waku/waku/v2/peermanager.(*PeerManager).Start
	/home/jenkins/workspace/status-go_prs_tests_PR-4020/vendor/github.com/waku-org/go-waku/waku/v2/peermanager/peer_manager.go:127 +0xc5

Probably caused by #759

@chaitanyaprem
Copy link
Collaborator

Testing this in status-im/status-go#4020 gives

panic: runtime error: slice bounds out of range [2:1]
goroutine 1232 [running]:
github.com/waku-org/go-waku/waku/v2/peerstore.(*WakuPeerstoreImpl).RemovePubSubTopic(0xc00117d290, {0xc001459560, 0x27}, {0x0?, 0x1a?})
	/home/jenkins/workspace/status-go_prs_tests_PR-4020/vendor/github.com/waku-org/go-waku/waku/v2/peerstore/waku_peer_store.go:174 +0x1f0
github.com/waku-org/go-waku/waku/v2/peermanager.(*PeerManager).peerEventLoop(0xc000716a20, {0x1669418, 0xc000541ac0})
	/home/jenkins/workspace/status-go_prs_tests_PR-4020/vendor/github.com/waku-org/go-waku/waku/v2/peermanager/peer_manager.go:109 +0x25c
created by github.com/waku-org/go-waku/waku/v2/peermanager.(*PeerManager).Start
	/home/jenkins/workspace/status-go_prs_tests_PR-4020/vendor/github.com/waku-org/go-waku/waku/v2/peermanager/peer_manager.go:127 +0xc5

Probably caused by #759

Nope, i think i figured out the issue for this. Handled in commit 0377ce8
Was a silly issue.
Let me push a fix this to master.

@chaitanyaprem
Copy link
Collaborator

You can try with this branch once and confirm

#765

@vitvly vitvly mentioned this pull request Sep 22, 2023
Merged
@vitvly
Copy link
Contributor Author

vitvly commented Sep 22, 2023

@chaitanyaprem sorry i've just noticed your comments here, left more context in #766. I was able to check that this fixes the issue. Can you please review other changes in this specific PR?

richard-ramos
richard-ramos approved these changes Sep 22, 2023
View reviewed changes
Copy link
Member

@richard-ramos richard-ramos left a comment

Choose a reason for hiding this comment

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

LGTM.
If you merge this PR, please squash your commits, and use the prefix feat: or fix: to follow the commit convention we're using in go-waku

waku/v2/peerstore/waku_peer_store.go Outdated Show resolved Hide resolved
waku/v2/protocol/filter/client.go Outdated Show resolved Hide resolved
waku/v2/protocol/filter/client.go Show resolved Hide resolved
waku/v2/protocol/filter/client.go Show resolved Hide resolved
waku/v2/protocol/filter/client.go Outdated Show resolved Hide resolved
@vitvly
Copy link
Contributor Author

vitvly commented Sep 22, 2023

allow creating a new Subscription for same (peer, contentFilter) pairs (this was mistakenly assumed to be wrong and thus removed in #750)

Out of curiosity, unable to understand a use-case where we would want something like this. Wouldn't this cause stale older subscriptions to remain and references lost to them in the filterClient?

Good question! We had a meeting earlier on with @cammellos and @richard-ramos on this (results: status-im/status-go#4042 (comment)).
In status-go a Filter entity is actually a tuple of (encryption_keys, contentFilter). Meaning, it is possible to subscribe to the same contentFilter, but with different encryption keys.
This shouldn't create stale references.

@vitvly vitvly force-pushed the refactor/filter-unsubscribe branch from 7d77b67 to 64f3da6 Compare September 22, 2023 13:49
feat: change UnsubscribeWithSubscription so that it's single sub-spec…
915f77f 
…ific

Also merge FilterSubscribe and FilterUnsubscribe options/params
@vitvly vitvly force-pushed the refactor/filter-unsubscribe branch from 391b05b to 915f77f Compare September 22, 2023 14:48
@vitvly vitvly merged commit 16ec225 into master Sep 22, 2023
1 check passed
@vitvly vitvly deleted the refactor/filter-unsubscribe branch September 22, 2023 14:53
@vitvly vitvly mentioned this pull request Sep 25, 2023
Closed
@chaitanyaprem chaitanyaprem mentioned this pull request Oct 19, 2023
Closed
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richard-ramos richard-ramos richard-ramos approved these changes

@chaitanyaprem chaitanyaprem Awaiting requested review from chaitanyaprem chaitanyaprem is a code owner

Assignees

@vitvly vitvly

Labels
None yet
Projects
Waku
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vitvly @status-im-auto @chaitanyaprem @richard-ramos