Skip to content

Commit 58855e8

Browse files
showelltimabbott
authored andcommitted
refactor: Remove maybe_clear_subscribers().
The maybe_clear_subscribers() function was an artifact of when we used to attach subscribers to the "sub" records in stream_data.js. I think it was basically a refactoring shim, and due to some other recent cleanup, it was only used in test code. We also change how we validate stream ids. Going forward, peer_data just looks up stream_ids with the normal stream_data API when it's trying to warn about rogue stream_ids coming in. As I alluded to in an earlier commit, some of the warning code here might be overly defensive, but at least it's pretty self-contained.
1 parent e44e48e commit 58855e8

File tree

4 files changed

+13
-15
lines changed

4 files changed

+13
-15
lines changed

frontend_tests/node_tests/compose.js

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -972,6 +972,8 @@ run_test("warn_if_private_stream_is_linked", () => {
972972
stream_id: 100,
973973
name: "Denmark",
974974
};
975+
stream_data.add_sub(denmark);
976+
975977
peer_data.set_subscribers(denmark.stream_id, [1, 2, 3]);
976978

977979
function test_noop_case(invite_only) {

frontend_tests/node_tests/peer_data.js

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -240,7 +240,11 @@ run_test("get_subscriber_count", () => {
240240

241241
run_test("is_subscriber_subset", () => {
242242
function make_sub(stream_id, user_ids) {
243-
const sub = {stream_id};
243+
const sub = {
244+
stream_id,
245+
name: `stream ${stream_id}`,
246+
};
247+
stream_data.add_sub(sub);
244248
peer_data.set_subscribers(sub.stream_id, user_ids);
245249
return sub;
246250
}

static/js/peer_data.js

Lines changed: 6 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,6 @@ const {LazySet} = require("./lazy_set");
22
const people = require("./people");
33

44
// This maps a stream_id to a LazySet of user_ids who are subscribed.
5-
// We maintain the invariant that this has keys for all all stream_ids
6-
// that we track in the other data structures. We intialize it during
7-
// clear_subscriptions.
85
let stream_subscribers;
96

107
function assert_number(id) {
@@ -14,14 +11,17 @@ function assert_number(id) {
1411
}
1512

1613
function get_user_set(stream_id) {
17-
assert_number(stream_id);
18-
1914
// This is an internal function to get the LazySet of users.
2015
// We create one on the fly as necessary, but we warn in that case.
16+
assert_number(stream_id);
17+
18+
if (!stream_data.get_sub_by_id(stream_id)) {
19+
blueslip.warn("We called get_user_set for an untracked stream: " + stream_id);
20+
}
21+
2122
let subscribers = stream_subscribers.get(stream_id);
2223

2324
if (subscribers === undefined) {
24-
blueslip.warn("We called get_user_set for an untracked stream: " + stream_id);
2525
subscribers = new LazySet([]);
2626
stream_subscribers.set(stream_id, subscribers);
2727
}
@@ -33,12 +33,6 @@ export function clear() {
3333
stream_subscribers = new Map();
3434
}
3535

36-
export function maybe_clear_subscribers(stream_id) {
37-
if (!stream_subscribers.has(stream_id)) {
38-
set_subscribers(stream_id, []);
39-
}
40-
}
41-
4236
export function is_subscriber_subset(stream_id1, stream_id2) {
4337
const sub1_set = get_user_set(stream_id1);
4438
const sub2_set = get_user_set(stream_id2);

static/js/stream_data.js

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -211,8 +211,6 @@ exports.add_sub = function (sub) {
211211
// This function is currently used only by tests.
212212
// We use create_sub_from_server_data at page load.
213213
// We use create_streams for new streams in live-update events.
214-
215-
peer_data.maybe_clear_subscribers(sub.stream_id);
216214
stream_info.set(sub.name, sub);
217215
subs_by_stream_id.set(sub.stream_id, sub);
218216
};

0 commit comments

Comments
 (0)