I'm checking what's wrong with the Android and iOS builds (maybe the protobuf changes broke some old client-side API code). Requesting review to unblock it for possible comments.

Herald added a reviewer: • karol. · View Herald TranscriptSep 19 2022, 6:39 AM

would be nice to have a test case that the online behavior is being correctly updated.

• max removed a parent revision: D5165: [services] Tunnelbroker - Add `updateSessionItemIsOnline` database method.Sep 20 2022, 6:21 AM

This is rather strange: for each get request we have an infinite loop of ping - is this the right approach?

services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
265–269	In case of an error, maybe we should also set the status to offline?

This revision now requires changes to proceed.Sep 20 2022, 6:59 AM

• max mentioned this in D5174: [services] Tunnelbroker - Add ping implementation to the `Get` handler.Sep 21 2022, 6:17 AM

Rebasing on the parent changes.

• max marked an inline comment as done.Sep 21 2022, 6:40 AM

• max added inline comments.

services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
265–269	In case of an error, maybe we should also set the status to offline? In the latest update we are switching the device as Online in the handler after we did a session checks. We are switching it as Offline in: When the write failed and we are switching the `std::atomic_bool writerIsReady` to false; When the exception occurred; When the unexpected error occured.

In D5175#152307, @tomek wrote:

This is rather strange: for each get request we have an infinite loop of ping - is this the right approach?

Yes, we are sending pings to make sure the device is Online, in case the write failed we are exiting from the loop and thread and considering the device as Offline.

In D5175#151890, @jon wrote:

would be nice to have a test case that the online behavior is being correctly updated.

Speaking of the unit tests we already have tests in D5166. To test how this algorithm behaves it can be tested by the isolated test using my repo on which I've tested this approach.
I don't think we can do such automated integration tests now (connect client, drop the connection, check the database status). So it can be checked manually, but would be cool to have it.

Harbormaster completed remote builds in B12346: Diff 16948.Sep 21 2022, 6:47 AM

• jon accepted this revision.Sep 22 2022, 11:29 AM

I'm still not sure if having this logic in get is the right approach. Added a comment about it in D5174 but accepting this diff because it looks ok

This revision is now accepted and ready to land.Sep 23 2022, 4:41 AM

Resolving merge conflicts.

In D5175#153316, @tomek wrote:

I'm still not sure if having this logic in get is the right approach. Added a comment about it in D5174 but accepting this diff because it looks ok

After the logic explanation in D5174 Tomek accepted the D5174, so I'm landing this one too.

This revision was landed with ongoing or failed builds.Sep 23 2022, 2:07 PM

Closed by commit rCOMM1fa7b64d2d6b: [services] Tunnelbroker - Update device online status in `Get` handler (authored by • max). · Explain Why

This revision was automatically updated to reflect the committed changes.

• max added a commit: rCOMM1fa7b64d2d6b: [services] Tunnelbroker - Update device online status in `Get` handler.

Harbormaster completed remote builds in B12430: Diff 17068.Sep 23 2022, 2:09 PM