Page MenuHomePhabricator

[lib] confirm Tunnelbroker messages after processing
ClosedPublic

Authored by kamil on Apr 23 2024, 4:27 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 7, 2:27 AM
Unknown Object (File)
Oct 15 2024, 7:25 PM
Unknown Object (File)
Oct 15 2024, 7:25 PM
Unknown Object (File)
Oct 15 2024, 7:25 PM
Unknown Object (File)
Oct 15 2024, 7:25 PM
Unknown Object (File)
Oct 15 2024, 7:25 PM
Unknown Object (File)
Oct 15 2024, 7:25 PM
Unknown Object (File)
Oct 15 2024, 7:25 PM
Subscribers

Details

Summary

We want to confirm message after processing it.

Depends on D11732

Test Plan

Add sleep and make sure message is confirmed after message was processed.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

kamil held this revision as a draft.
lib/tunnelbroker/tunnelbroker-context.js
273 ↗(On Diff #39394)

we get the incorrect message - so we just confirm it so Tunnelbroker will remove it from it's queue

279 ↗(On Diff #39394)

invalid Tunnelbroker message - this could happen when the client has an older version and is not capable of processing recently added new messages, we can consider informing the user to somehow update the app, but for now, confirming is the best we can do

290 ↗(On Diff #39394)

peerToPeerMessageHandler is designed so that it will not throw, we confirm regardless of success or error while processing

kamil published this revision for review.Apr 23 2024, 6:43 AM
lib/tunnelbroker/tunnelbroker-context.js
279 ↗(On Diff #39394)

Should we add an in-code comment describing this scenario?

tomek added inline comments.
lib/tunnelbroker/tunnelbroker-context.js
290 ↗(On Diff #39394)

What if socket.current is falsy? When can this happen? Should we try confirming again after it becomes truthy?

This revision is now accepted and ready to land.Apr 29 2024, 12:36 AM
lib/tunnelbroker/tunnelbroker-context.js
290 ↗(On Diff #39394)

socket.current is falsy then this code shouldn't be executed as it's called only as a response to the received message - when socket.current is defined, ? is only to satisfy flow

lib/tunnelbroker/tunnelbroker-context.js
290 ↗(On Diff #39394)

that's not true - I need to update this diff, ignore my comment

kamil requested review of this revision.Apr 29 2024, 4:57 AM
tomek added inline comments.
lib/tunnelbroker/tunnelbroker-context.js
310–311

Should we also check localSocketSessionCounter === socketSessionCounter.current here?

This revision is now accepted and ready to land.Apr 29 2024, 5:32 AM

add missing check

lib/tunnelbroker/tunnelbroker-context.js
310–311

yes, thanks fo catchinng

This revision was landed with ongoing or failed builds.Apr 29 2024, 6:04 AM
This revision was automatically updated to reflect the committed changes.