Page MenuHomePhabricator
Differential D5207

[services] Tunnelbroker - Update sending of the `newNotifyTokenRequired` in `Get` stream request
ClosedPublic
Actions

Authored by • max on Sep 21 2022, 4:06 PM.
Tags
None
Referenced Files
F3356924: D5207.id17063.diff
Sat, Nov 23, 9:31 PM
F3356760: D5207.id17029.diff
Sat, Nov 23, 8:35 PM
F3356358: D5207.diff
Sat, Nov 23, 6:34 PM
Unknown Object (File)
Thu, Nov 21, 4:44 AM
Unknown Object (File)
Sat, Oct 26, 2:30 AM
Unknown Object (File)
Oct 23 2024, 1:53 PM
Unknown Object (File)
Oct 23 2024, 1:43 PM
Unknown Object (File)
Oct 9 2024, 8:44 PM
View All Files
Subscribers
• abosh
ashoat
atul
tomek

Details

Reviewers
• jon
tomek
Commits
rCOMM3e557c0336aa: [services] Tunnelbroker - Update sending of the `newNotifyTokenRequired` in…
Summary

This diff introduces sending of the newNotifyTokenRequired to the client in Get stream request.
When the device has an invalid or expired notification token it's empty in the database and we are requesting a new token from the client using the newNotifyTokenRequired response.
The client gets back with the new token in Get stream request when starts listening for new messages (getting messages) from the Tunnelbroker (ref to D5206).

This is a part of the stack.
This approach was used keeping in mind the transition to the bidirectional stream soon.

Related Linear task: ENG-1782 and ENG-1816

Test Plan

Successfully built the service and passed unit and integration tests.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

• max created this revision.Sep 21 2022, 4:06 PM
• max held this revision as a draft.
Herald added a reviewer: • jon. · View Herald TranscriptSep 21 2022, 4:06 PM
Herald added subscribers: • abosh, atul, tomek, ashoat. · View Herald Transcript
• max mentioned this in D5206: [services] Tunnelbroker - Update handling of the `newNotifyToken` in `Get` stream request.Sep 21 2022, 4:08 PM
• max published this revision for review.Sep 21 2022, 4:11 PM
• max edited the summary of this revision. (Show Details)
• max edited the test plan for this revision. (Show Details)
• max added a reviewer: tomek.
• max added a parent revision: D5206: [services] Tunnelbroker - Update handling of the `newNotifyToken` in `Get` stream request.
Harbormaster completed remote builds in B12360: Diff 16969.Sep 21 2022, 4:16 PM
• max mentioned this in D5210: [services] Tunnelbroker - Updating `Send` handler with using of the Rust notification library.Sep 22 2022, 5:12 AM
• jon accepted this revision.Sep 22 2022, 4:56 PM
tomek accepted this revision.Sep 23 2022, 5:11 AM
tomek added inline comments.
services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
199–201 ↗(On Diff #16969)

Is it really failed? I guess it depends, because sending a response where we inform that a new token is required sounds like a success. But making it a failure also makes some sense. So up to you.

This revision is now accepted and ready to land.Sep 23 2022, 5:11 AM
ashoat added inline comments.Sep 23 2022, 5:20 AM
services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
199–201 ↗(On Diff #16969)

We should not fail. Some devices will not have a notifyToken

tomek requested changes to this revision.Sep 23 2022, 5:21 AM
This revision now requires changes to proceed.Sep 23 2022, 5:21 AM
• max planned changes to this revision.Sep 23 2022, 5:53 AM
• max updated this revision to Diff 17029.Sep 23 2022, 7:37 AM

Removing return of error if the device token is empty.

• max marked 2 inline comments as done.Sep 23 2022, 7:38 AM
• max added inline comments.
services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
199–201 ↗(On Diff #16969)

Is it really failed? I guess it depends, because sending a response where we inform that a new token is required sounds like a success. But making it a failure also makes some sense. So up to you.

We should not fail. Some devices will not have a notifyToken

Makes sense to remove returning of the error here.
I've removed the return of error here.

Harbormaster failed remote builds in B12410: Diff 17029!Sep 23 2022, 7:44 AM
tomek accepted this revision.Sep 23 2022, 7:52 AM
tomek added inline comments.
services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
188–189 ↗(On Diff #17029)

Shouldn't we fetch the session item again after this update?

194–198 ↗(On Diff #17029)

Shouldn't we clear the response after it is sent?

This revision is now accepted and ready to land.Sep 23 2022, 7:52 AM
Harbormaster completed remote builds in B12410: Diff 17029.Sep 23 2022, 11:14 AM
• max mentioned this in rCOMMa206a33acdc2: [services] Tunnelbroker - Updating `Send` handler with using of the Rust….Sep 23 2022, 12:46 PM
• max mentioned this in rCOMM8f4cc18c8948: [services] Tunnelbroker - Update handling of the `newNotifyToken` in `Get`….Sep 23 2022, 12:49 PM
• max updated this revision to Diff 17063.Sep 23 2022, 1:04 PM
• max marked 2 inline comments as done.

Adding of request cleaning.
Adding of session re-fetching after token update.
Resolving merge conflict.

• max marked an inline comment as done.Sep 23 2022, 1:05 PM
• max added inline comments.
services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
188–189 ↗(On Diff #17029)

Shouldn't we fetch the session item again after this update?

Nice catch! I've added refetching of the session after the token update.

194–198 ↗(On Diff #17029)

Shouldn't we clear the response after it is sent?

Yes, that makes sense! I've added the .Clean() here.

This revision was landed with ongoing or failed builds.Sep 23 2022, 1:13 PM
Closed by commit rCOMM3e557c0336aa: [services] Tunnelbroker - Update sending of the `newNotifyTokenRequired` in… (authored by • max). · Explain Why
This revision was automatically updated to reflect the committed changes.
• max marked an inline comment as done.
• max added a commit: rCOMM3e557c0336aa: [services] Tunnelbroker - Update sending of the `newNotifyTokenRequired` in….
Harbormaster completed remote builds in B12427: Diff 17063.Sep 23 2022, 1:14 PM
tomek added inline comments.Sep 26 2022, 1:33 AM
services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
188–189 ↗(On Diff #17029)

The fact that this was caught during the review suggests that this functionality wasn't tested thoroughly. Could you test if setting new token works properly? Have you checked it before this change?

• max marked an inline comment as done.Sep 26 2022, 1:37 AM
• max added inline comments.
services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
188–189 ↗(On Diff #17029)

The fact that this was caught during the review suggests that this functionality wasn't tested thoroughly. Could you test if setting new token works properly? Have you checked it before this change?

Sure, I'll re-test all the API flow and will re-check it now (I'm testing now using httpyac script ) The fact that this condition was lost in this certain diff was the result of the code merging from my side.

Thanks, @tomek !

• max marked an inline comment as done.Sep 26 2022, 1:39 AM
• max added inline comments.
services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
188–189 ↗(On Diff #17029)

The fact that this was caught during the review suggests that this functionality wasn't tested thoroughly. Could you test if setting new token works properly? Have you checked it before this change?

Sure, I'll re-test all the API flow and will re-check it now (I'm testing now using httpyac script ) The fact that this condition was lost in this certain diff was the result of the code merging from my side.

Thanks, @tomek !

By the way, upcoming integration tests in CI will catch such errors.

Revision Contents
Changeset List

PathSize
services/
tunnelbroker/
src/
Service/
26 lines

Diff 17064

View Options

services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp

Loading...