-
Notifications
You must be signed in to change notification settings - Fork 564
Update FakeChannel.await_result(...) to drive async Rust (Tokio runtime/thread pool)
#19879
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
Changes from all commits
f5710de
bb9e266
0fb3722
c10f8cb
0281dd2
842fd59
3565a7f
7c19a47
622ec3d
ade5ef6
60a1507
81cbffa
c349a33
b64e067
7a1bd83
e9d6aa3
4507561
5c4a3b3
21af1ca
62bb99a
4107108
15dd368
609f7a0
c08f96f
a7d6999
fbeb0cc
5956704
0042368
27b3366
d4f4ddd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Update `HomeserverTestCase.get_success(...)` and friends to drive async Rust (Tokio runtime/thread pool). | ||
|
MadLittleMods marked this conversation as resolved.
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -117,7 +117,7 @@ async def check_auth_blocking( | |
| # If the user is already part of the MAU cohort or a trial user | ||
| if user_id: | ||
| timestamp = await self.store.user_last_seen_monthly_active(user_id) | ||
| if timestamp: | ||
| if timestamp is not None: | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixing a flawed check ( |
||
| return | ||
|
|
||
| is_trial = await self.store.is_trial_user(user_id) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,6 +37,7 @@ | |
| from synapse.storage.databases.main.events_worker import EventMetadata | ||
| from synapse.types import JsonDict, ReadReceipt | ||
| from synapse.util.clock import Clock | ||
| from synapse.util.duration import Duration | ||
|
|
||
| from tests.unittest import HomeserverTestCase | ||
|
|
||
|
|
@@ -517,6 +518,24 @@ async def record_transaction( | |
| self.edus.extend(data["edus"]) | ||
| return {} | ||
|
|
||
| def wait_for_device_list_updates_to_be_sent(self) -> None: | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added |
||
| """ | ||
| Wait for the device list update EDU's to get pushed out over federation | ||
|
|
||
| For example, each login does a fire-and-forget (`LoginRestServlet` -> | ||
| `register_device` -> `notify_device_update` -> `handle_new_device_update` -> | ||
| `send_device_messages(hosts, immediate=False)`) which adds to the | ||
| `_DestinationWakeupQueue` which has a background process that sends depending on | ||
| how `federation_rr_transactions_per_room_per_second` is configured. | ||
|
|
||
| The default `federation_rr_transactions_per_room_per_second` is `50` (1s/50 -> | ||
| 0.02s) | ||
| """ | ||
| self.reactor.advance( | ||
| 1.0 | ||
| / self.hs.config.ratelimiting.federation_rr_transactions_per_room_per_second | ||
| ) | ||
|
|
||
| def test_send_device_updates(self) -> None: | ||
| """Basic case: each device update should result in an EDU""" | ||
| # create a device | ||
|
|
@@ -527,9 +546,7 @@ def test_send_device_updates(self) -> None: | |
| self.assertEqual(len(self.edus), 1) | ||
| stream_id = self.check_device_update_edu(self.edus.pop(0), u1, "D1", None) | ||
|
|
||
| # We queue up device list updates to be sent over federation, so we | ||
| # advance to clear the queue. | ||
| self.reactor.advance(1) | ||
| self.wait_for_device_list_updates_to_be_sent() | ||
|
|
||
| # a second call should produce no new device EDUs | ||
| self.get_success( | ||
|
|
@@ -568,7 +585,7 @@ def test_dont_send_device_updates_for_remote_users(self) -> None: | |
| ) | ||
| ) | ||
|
|
||
| self.reactor.advance(1) | ||
| self.wait_for_device_list_updates_to_be_sent() | ||
|
|
||
| # We shouldn't see an EDU for that update | ||
| self.assertEqual(self.edus, []) | ||
|
|
@@ -590,6 +607,8 @@ def test_upload_signatures(self) -> None: | |
| self.login(u1, "pass", device_id="D1") | ||
| self.login(u1, "pass", device_id="D2") | ||
|
|
||
| self.wait_for_device_list_updates_to_be_sent() | ||
|
|
||
| # expect two edus | ||
| self.assertEqual(len(self.edus), 2) | ||
| stream_id: int | None = None | ||
|
|
@@ -600,9 +619,7 @@ def test_upload_signatures(self) -> None: | |
| device1_signing_key = self.generate_and_upload_device_signing_key(u1, "D1") | ||
| device2_signing_key = self.generate_and_upload_device_signing_key(u1, "D2") | ||
|
|
||
| # We queue up device list updates to be sent over federation, so we | ||
| # advance to clear the queue. | ||
| self.reactor.advance(1) | ||
| self.wait_for_device_list_updates_to_be_sent() | ||
|
|
||
| # expect two more edus | ||
| self.assertEqual(len(self.edus), 2) | ||
|
|
@@ -637,9 +654,7 @@ def test_upload_signatures(self) -> None: | |
| e2e_handler.upload_signing_keys_for_user(u1, cross_signing_keys) | ||
| ) | ||
|
|
||
| # We queue up device list updates to be sent over federation, so we | ||
| # advance to clear the queue. | ||
| self.reactor.advance(1) | ||
| self.wait_for_device_list_updates_to_be_sent() | ||
|
|
||
| # expect signing key update edu | ||
| self.assertEqual(len(self.edus), 2) | ||
|
|
@@ -662,9 +677,7 @@ def test_upload_signatures(self) -> None: | |
| ) | ||
| self.assertEqual(ret["failures"], {}) | ||
|
|
||
| # We queue up device list updates to be sent over federation, so we | ||
| # advance to clear the queue. | ||
| self.reactor.advance(1) | ||
| self.wait_for_device_list_updates_to_be_sent() | ||
|
|
||
| # expect two edus, in one or two transactions. We don't know what order the | ||
| # devices will be updated. | ||
|
|
@@ -689,9 +702,7 @@ def test_delete_devices(self) -> None: | |
| self.login("user", "pass", device_id="D2") | ||
| self.login("user", "pass", device_id="D3") | ||
|
|
||
| # We queue up device list updates to be sent over federation, so we | ||
| # advance to clear the queue. | ||
| self.reactor.advance(1) | ||
| self.wait_for_device_list_updates_to_be_sent() | ||
|
|
||
| # expect three edus | ||
| self.assertEqual(len(self.edus), 3) | ||
|
|
@@ -702,9 +713,7 @@ def test_delete_devices(self) -> None: | |
| # delete them again | ||
| self.get_success(self.device_handler.delete_devices(u1, ["D1", "D2", "D3"])) | ||
|
|
||
| # We queue up device list updates to be sent over federation, so we | ||
| # advance to clear the queue. | ||
| self.reactor.advance(1) | ||
| self.wait_for_device_list_updates_to_be_sent() | ||
|
|
||
| # expect three edus, in an unknown order | ||
| self.assertEqual(len(self.edus), 3) | ||
|
|
@@ -730,15 +739,18 @@ def test_unreachable_server(self) -> None: | |
| # create devices | ||
| u1 = self.register_user("user", "pass") | ||
| self.login("user", "pass", device_id="D1") | ||
| # Wait some time in between each device list update as we want each of them to | ||
| # be attempted to be sent in their own transaction | ||
| self.reactor.advance(Duration(seconds=1).as_secs()) | ||
|
Comment on lines
+742
to
+744
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The main fixes to these tests was adding some time in between each device list update so they are sent in their own transaction. Tests assert Previously, we advanced |
||
| self.login("user", "pass", device_id="D2") | ||
| self.reactor.advance(Duration(seconds=1).as_secs()) | ||
| self.login("user", "pass", device_id="D3") | ||
| self.reactor.advance(Duration(seconds=1).as_secs()) | ||
|
|
||
| # delete them again | ||
| self.get_success(self.device_handler.delete_devices(u1, ["D1", "D2", "D3"])) | ||
|
|
||
| # We queue up device list updates to be sent over federation, so we | ||
| # advance to clear the queue. | ||
| self.reactor.advance(1) | ||
| self.wait_for_device_list_updates_to_be_sent() | ||
|
|
||
| self.assertGreaterEqual(mock_send_txn.call_count, 4) | ||
|
|
||
|
|
@@ -748,9 +760,7 @@ def test_unreachable_server(self) -> None: | |
| self.hs.get_federation_sender().send_device_messages(["host2"]) | ||
| ) | ||
|
|
||
| # We queue up device list updates to be sent over federation, so we | ||
| # advance to clear the queue. | ||
| self.reactor.advance(1) | ||
| self.wait_for_device_list_updates_to_be_sent() | ||
|
|
||
| # for each device, there should be a single update | ||
| self.assertEqual(len(self.edus), 3) | ||
|
|
@@ -777,16 +787,20 @@ def test_prune_outbound_device_pokes1(self) -> None: | |
| # create devices | ||
| u1 = self.register_user("user", "pass") | ||
| self.login("user", "pass", device_id="D1") | ||
| # Wait some time in between each device list update as we want each of them to | ||
| # be attempted to be sent in their own transaction | ||
| self.reactor.advance(Duration(seconds=1).as_secs()) | ||
| self.login("user", "pass", device_id="D2") | ||
| self.reactor.advance(Duration(seconds=1).as_secs()) | ||
| self.login("user", "pass", device_id="D3") | ||
| self.reactor.advance(Duration(seconds=1).as_secs()) | ||
|
|
||
| # delete them again | ||
| self.get_success(self.device_handler.delete_devices(u1, ["D1", "D2", "D3"])) | ||
|
|
||
| # We queue up device list updates to be sent over federation, so we | ||
| # advance to clear the queue. | ||
| self.reactor.advance(1) | ||
| self.wait_for_device_list_updates_to_be_sent() | ||
|
|
||
| # Ensure that we tried sending the device list update EDU's out | ||
| self.assertGreaterEqual(mock_send_txn.call_count, 4) | ||
|
|
||
| # run the prune job | ||
|
|
@@ -801,9 +815,7 @@ def test_prune_outbound_device_pokes1(self) -> None: | |
| self.hs.get_federation_sender().send_device_messages(["host2"]) | ||
| ) | ||
|
|
||
| # We queue up device list updates to be sent over federation, so we | ||
| # advance to clear the queue. | ||
| self.reactor.advance(1) | ||
| self.wait_for_device_list_updates_to_be_sent() | ||
|
|
||
| # there should be a single update for this user. | ||
| self.assertEqual(len(self.edus), 1) | ||
|
|
@@ -835,15 +847,17 @@ def test_prune_outbound_device_pokes2(self) -> None: | |
| mock_send_txn.side_effect = AssertionError("fail") | ||
|
|
||
| self.login("user", "pass", device_id="D2") | ||
| # Wait some time in between each device list update as we want each of them to | ||
| # be attempted to be sent in their own transaction | ||
| self.reactor.advance(Duration(seconds=1).as_secs()) | ||
| self.login("user", "pass", device_id="D3") | ||
|
|
||
| # We queue up device list updates to be sent over federation, so we | ||
| # advance to clear the queue. | ||
| self.reactor.advance(1) | ||
| self.reactor.advance(Duration(seconds=1).as_secs()) | ||
|
|
||
| # delete them again | ||
| self.get_success(self.device_handler.delete_devices(u1, ["D1", "D2", "D3"])) | ||
|
|
||
| self.wait_for_device_list_updates_to_be_sent() | ||
|
|
||
| self.assertGreaterEqual(mock_send_txn.call_count, 3) | ||
|
|
||
| # run the prune job | ||
|
|
@@ -858,9 +872,7 @@ def test_prune_outbound_device_pokes2(self) -> None: | |
| self.hs.get_federation_sender().send_device_messages(["host2"]) | ||
| ) | ||
|
|
||
| # We queue up device list updates to be sent over federation, so we | ||
| # advance to clear the queue. | ||
| self.reactor.advance(1) | ||
| self.wait_for_device_list_updates_to_be_sent() | ||
|
|
||
| # ... and we should get a single update for this user. | ||
| self.assertEqual(len(self.edus), 1) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -564,9 +564,16 @@ def test_wait_for_sync_token(self) -> None: | |
| ) | ||
| # Block for 10 seconds to make `notifier.wait_for_stream_token(from_token)` | ||
| # timeout | ||
| # | ||
| # First, block for *almost* 10 seconds to make sure we are | ||
| # `notifier.wait_for_stream_token(from_token)` | ||
| with self.assertRaises(TimedOutException): | ||
| channel.await_result(timeout_ms=9900) | ||
| channel.await_result(timeout_ms=200) | ||
| # Then wait for the rest of the 10 second timeout, 9900 + 500 > 10000 | ||
| # | ||
| # `notifier.wait_for_stream_token(from_token)` only checks every 500ms so we | ||
| # need to match that in order to make sure we hit the wake-up for sure. | ||
| channel.await_result(timeout_ms=500) | ||
|
Comment on lines
+574
to
+576
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Previously, this worked because Now I've updated the second wait to be 500ms to match |
||
| self.assertEqual(channel.code, 200, channel.json_body) | ||
|
|
||
| # We expect the next `pos` in the result to be the same as what we requested | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -175,13 +175,11 @@ def test_rendezvous_public(self) -> None: | |
| self.assertEqual(channel.code, 200) | ||
| rendezvous_id = channel.json_body["id"] | ||
| sequence_token = channel.json_body["sequence_token"] | ||
| expires_in_ms = channel.json_body["expires_in_ms"] | ||
| self.assertGreater(expires_in_ms, 0) | ||
| self.assertGreater(channel.json_body["expires_in_ms"], 0) | ||
|
|
||
| session_endpoint = rz_endpoint + f"/{rendezvous_id}" | ||
|
|
||
| # We can get the data back | ||
| # Advances clock by 100ms | ||
| channel = self.make_request( | ||
| "GET", | ||
| session_endpoint, | ||
|
|
@@ -191,10 +189,9 @@ def test_rendezvous_public(self) -> None: | |
| self.assertEqual(channel.code, 200) | ||
| self.assertEqual(channel.json_body["data"], "foo=bar") | ||
| self.assertEqual(channel.json_body["sequence_token"], sequence_token) | ||
| self.assertEqual(channel.json_body["expires_in_ms"], expires_in_ms - 100) | ||
| self.assertGreater(channel.json_body["expires_in_ms"], 0) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Had to touch this because We don't need to test exact And we don't care about testing it on subsequent requests either. This test has nothing to do with expiration. If we wanted to do this kind of thing, we should do something like the following: duration_between_requests = Duration(milliseconds=100)
# Advance by some known amount
self.reactor.advance(duration_between_requests.as_secs())
# ...
self.assertGreater(channel.json_body["expires_in_ms"], 0)
self.assertLessEqual(
channel.json_body["expires_in_ms"],
prev_expires_in_ms - duration_between_requests.as_millis(),
)
prev_expires_in_ms = channel.json_body["expires_in_ms"]Related discussion: #19539 (comment) |
||
|
|
||
| # We can update the data | ||
| # Advances clock by 100ms | ||
| channel = self.make_request( | ||
| "PUT", | ||
| session_endpoint, | ||
|
|
@@ -207,7 +204,6 @@ def test_rendezvous_public(self) -> None: | |
| new_sequence_token = channel.json_body["sequence_token"] | ||
|
|
||
| # If we try to update it again with the old etag, it should fail | ||
| # Advances clock by 100ms | ||
| channel = self.make_request( | ||
| "PUT", | ||
| session_endpoint, | ||
|
|
@@ -221,7 +217,6 @@ def test_rendezvous_public(self) -> None: | |
| ) | ||
|
|
||
| # We should get the updated data | ||
| # Advances clock by 100ms | ||
| channel = self.make_request( | ||
| "GET", | ||
| session_endpoint, | ||
|
|
@@ -231,7 +226,7 @@ def test_rendezvous_public(self) -> None: | |
| self.assertEqual(channel.code, 200) | ||
| self.assertEqual(channel.json_body["data"], "foo=baz") | ||
| self.assertEqual(channel.json_body["sequence_token"], new_sequence_token) | ||
| self.assertEqual(channel.json_body["expires_in_ms"], expires_in_ms - 400) | ||
| self.assertGreater(channel.json_body["expires_in_ms"], 0) | ||
|
|
||
| # We can delete the data | ||
| channel = self.make_request( | ||
|
|
@@ -376,8 +371,7 @@ def test_rendezvous_requires_authentication(self) -> None: | |
| self.assertEqual(channel.code, 200) | ||
| rendezvous_id = channel.json_body["id"] | ||
| sequence_token = channel.json_body["sequence_token"] | ||
| expires_in_ms = channel.json_body["expires_in_ms"] | ||
| self.assertEqual(expires_in_ms, 120000) | ||
| self.assertGreater(channel.json_body["expires_in_ms"], 0) | ||
|
|
||
| session_endpoint = rz_endpoint + f"/{rendezvous_id}" | ||
|
|
||
|
|
@@ -391,7 +385,7 @@ def test_rendezvous_requires_authentication(self) -> None: | |
| self.assertEqual(channel.code, 200) | ||
| self.assertEqual(channel.json_body["data"], "foo=bar") | ||
| self.assertEqual(channel.json_body["sequence_token"], sequence_token) | ||
| self.assertEqual(channel.json_body["expires_in_ms"], expires_in_ms - 100) | ||
| self.assertGreater(channel.json_body["expires_in_ms"], 0) | ||
|
|
||
| # We can update the data without authentication | ||
| channel = self.make_request( | ||
|
|
@@ -414,7 +408,7 @@ def test_rendezvous_requires_authentication(self) -> None: | |
| self.assertEqual(channel.code, 200) | ||
| self.assertEqual(channel.json_body["data"], "foo=baz") | ||
| self.assertEqual(channel.json_body["sequence_token"], new_sequence_token) | ||
| self.assertEqual(channel.json_body["expires_in_ms"], expires_in_ms - 300) | ||
| self.assertGreater(channel.json_body["expires_in_ms"], 0) | ||
|
|
||
| # We can delete the data without authentication | ||
| channel = self.make_request( | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same changelog as #19871 so they merge
We can consider
FakeChannel.await_result(...)one of the "friends" ofHomeserverTestCase.get_success(...)