Page MenuHomePhabricator

[lib] confirm Tunnelbroker messages after processing
ClosedPublic

Authored by kamil on Tue, Apr 23, 4:27 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, May 4, 1:57 PM
Unknown Object (File)
Fri, May 3, 11:13 PM
Unknown Object (File)
Fri, May 3, 9:44 PM
Unknown Object (File)
Fri, May 3, 9:24 PM
Unknown Object (File)
Thu, May 2, 11:41 PM
Unknown Object (File)
Wed, May 1, 1:57 PM
Unknown Object (File)
Mon, Apr 29, 2:14 AM
Unknown Object (File)
Sun, Apr 28, 8:56 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
Lint Not Applicable
Unit
Tests Not Applicable

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.Tue, Apr 23, 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.Mon, Apr 29, 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.Mon, Apr 29, 4:57 AM
tomek added inline comments.
lib/tunnelbroker/tunnelbroker-context.js
310–311 ↗(On Diff #39646)

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

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

add missing check

lib/tunnelbroker/tunnelbroker-context.js
310–311 ↗(On Diff #39646)

yes, thanks fo catchinng

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