Page MenuHomePhabricator
Differential D13228

[tunnelbroker] Add RPC for closing WebSocket sessions
ClosedPublic
Actions

Authored by bartek on Sep 2 2024, 11:49 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 15, 4:05 AM
Unknown Object (File)
Tue, Mar 11, 8:09 PM
Unknown Object (File)
Mar 5 2025, 7:01 AM
Unknown Object (File)
Feb 26 2025, 11:38 AM
Unknown Object (File)
Feb 26 2025, 11:38 AM
Unknown Object (File)
Feb 26 2025, 11:37 AM
Unknown Object (File)
Feb 25 2025, 1:39 AM
Unknown Object (File)
Feb 17 2025, 2:15 PM
View All 46 Files
Subscribers
tomek

Details

Reviewers
kamil
ashoat
Commits
rCOMM1146dc9cfddf: [tunnelbroker] Add RPC for closing WebSocket sessions
Summary

Address ENG-8217.

Added Tunnelbroker RPC that force-closes WebSocket session with given device ID (if present).
The message is not persisted - it makes no point of storing it if the device isn't connected to Tunnelbroker at the time.

Depends on D13227

Test Plan

Repeated test plan from D13227, now by calling the RPC

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

bartek created this revision.Sep 2 2024, 11:49 PM
bartek held this revision as a draft.
Herald added a subscriber: tomek. · View Herald TranscriptSep 2 2024, 11:49 PM
bartek added a child revision: D13229: [identity] Ask Tunnelbroker to terminate old device sessions.Sep 2 2024, 11:52 PM
Harbormaster completed remote builds in B31404: Diff 43850.Sep 3 2024, 12:04 AM
bartek published this revision for review.Sep 3 2024, 12:13 AM
bartek added inline comments.
services/tunnelbroker/src/grpc/mod.rs
91–95

Also noticed in the docs:

Setting the TTL to 0 causes messages to be expired upon reaching a queue unless they can be delivered to a consumer immediately. Thus this provides an alternative to the immediate publishing flag, which the RabbitMQ server does not support.

I considered this but decided to give it a short timeout (1000ms), just in case.

ashoat added a comment.Sep 4 2024, 11:22 PM

This is meant to be called by other services, right? After ENG-9129 is resolved, it should only be possible to access from other services?

ashoat accepted this revision.Sep 4 2024, 11:22 PM
bartek added a comment.Sep 5 2024, 3:59 AM
In D13228#373308, @ashoat wrote:

This is meant to be called by other services, right? After ENG-9129 is resolved, it should only be possible to access from other services?

Yes, this is right. Only services should be able to call it (namely Identity)

kamil accepted this revision.Sep 6 2024, 6:10 AM
This revision is now accepted and ready to land.Sep 6 2024, 6:10 AM
Closed by commit rCOMM1146dc9cfddf: [tunnelbroker] Add RPC for closing WebSocket sessions (authored by bartek). · Explain WhySep 9 2024, 6:15 AM
This revision was automatically updated to reflect the committed changes.
bartek added a commit: rCOMM1146dc9cfddf: [tunnelbroker] Add RPC for closing WebSocket sessions.

Revision Contents
Changeset List

PathSize
services/
tunnelbroker/
src/
grpc/
31 lines
shared/
protos/
9 lines

Diff 43850

View Options

services/tunnelbroker/src/grpc/mod.rs

Show First 20 Lines • Show All 396 Lines • ▼ Show 20 Lines async ensureAndroidPushNotifsEnabled() {
let [hasPermission] = permissionPromisesResult; let [hasPermission] = permissionPromisesResult;
const [, canRequestPermission] = permissionPromisesResult; const [, canRequestPermission] = permissionPromisesResult;
if (!hasPermission && canRequestPermission) { if (!hasPermission && canRequestPermission) {
const permissionResponse = await (async () => { const permissionResponse = await (async () => {
// We issue a call to sleep to match iOS behavior where prompt // We issue a call to sleep to match iOS behavior where prompt
// doesn't appear immediately but after logged-out modal disappears // doesn't appear immediately but after logged-out modal disappears
await sleep(10); await sleep(10);
await this.requestAndroidNotificationsPermission(); return await this.requestAndroidNotificationsPermission();
marcinUnsubmitted
Not Done
Inline Actions

We definitely should return something from this lambda since otherwise it is useless to assign it to variable.

When we find we don't have Android notif permissions, we ignore the result of requestAndroidNotificationsPermission. It seems like that would cause us to fail to register notif permissions on Android.

If we find that we don't have permissions then just calling await this.requestAndroidNotificationsPermission(); would result in a prompt asking for notifications permissions. However if the user grants those permissions then hasPermissions is still falsy (since promise returned nothing), so deviceToken will be set to null. Nevertheless permissions are actually granted byt the OS, so the state on the device and keyserver would heal itself on next render.

This differential fixes this case so that if user grants permissions correct state is achieved immediately without need for additional re-render to heal the state.

marcin: We definitely should return something from this lambda since otherwise it is useless to assign…
})(); })();
hasPermission = permissionResponse; hasPermission = permissionResponse;
} }
if (!hasPermission) { if (!hasPermission) {
this.failedToRegisterPushPermissionsAndroid(!canRequestPermission); this.failedToRegisterPushPermissionsAndroid(!canRequestPermission);
return; return;
} }
▲ Show 20 Lines • Show All 328 Lines • Show Last 20 Lines
View Options

shared/protos/tunnelbroker.proto

Show First 20 Lines • Show All 396 Lines • ▼ Show 20 Lines async ensureAndroidPushNotifsEnabled() {
let [hasPermission] = permissionPromisesResult; let [hasPermission] = permissionPromisesResult;
const [, canRequestPermission] = permissionPromisesResult; const [, canRequestPermission] = permissionPromisesResult;
if (!hasPermission && canRequestPermission) { if (!hasPermission && canRequestPermission) {
const permissionResponse = await (async () => { const permissionResponse = await (async () => {
// We issue a call to sleep to match iOS behavior where prompt // We issue a call to sleep to match iOS behavior where prompt
// doesn't appear immediately but after logged-out modal disappears // doesn't appear immediately but after logged-out modal disappears
await sleep(10); await sleep(10);
await this.requestAndroidNotificationsPermission(); return await this.requestAndroidNotificationsPermission();
marcinUnsubmitted
Not Done
Inline Actions

We definitely should return something from this lambda since otherwise it is useless to assign it to variable.

When we find we don't have Android notif permissions, we ignore the result of requestAndroidNotificationsPermission. It seems like that would cause us to fail to register notif permissions on Android.

If we find that we don't have permissions then just calling await this.requestAndroidNotificationsPermission(); would result in a prompt asking for notifications permissions. However if the user grants those permissions then hasPermissions is still falsy (since promise returned nothing), so deviceToken will be set to null. Nevertheless permissions are actually granted byt the OS, so the state on the device and keyserver would heal itself on next render.

This differential fixes this case so that if user grants permissions correct state is achieved immediately without need for additional re-render to heal the state.

marcin: We definitely should return something from this lambda since otherwise it is useless to assign…
})(); })();
hasPermission = permissionResponse; hasPermission = permissionResponse;
} }
if (!hasPermission) { if (!hasPermission) {
this.failedToRegisterPushPermissionsAndroid(!canRequestPermission); this.failedToRegisterPushPermissionsAndroid(!canRequestPermission);
return; return;
} }
▲ Show 20 Lines • Show All 328 Lines • Show Last 20 Lines