Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Change SessionHolder to a weak ref #18397

Closed
wants to merge 1 commit into from
Closed

Change SessionHolder to a weak ref #18397

wants to merge 1 commit into from

Conversation

kghost
Copy link
Contributor

@kghost kghost commented May 12, 2022 •
edited
Loading

Problem

Rework SessionHandle/SessionHolder with the idea by @mrjerryjohns

Change overview

SessionHolder do not change ref count any more

Since SessionHolder is a weak ref now, In PairingSession, we have to use a handle to grab both unauthenticated session and secure session.

TODO:

  • Rename SessionHolder to SessionWeakPtr
  • UnauthenticatedSession is not longer LRU, it can be released immediately once its refcount becomes 0

Testing

Passed unit-tests

@restyled-io restyled-io bot mentioned this pull request May 12, 2022
Closed
@mrjerryjohns mrjerryjohns mentioned this pull request May 18, 2022
Closed
@msandstedt msandstedt self-requested a review May 20, 2022 20:37
@kghost kghost force-pushed the session-2 branch 2 times, most recently from 2bffbcd to 8fcb4de Compare May 24, 2022 17:01
@kghost kghost changed the title [PoC] Change SessionHolder to a weak ref Change SessionHolder to a weak ref May 24, 2022
@kghost kghost marked this pull request as ready for review May 24, 2022 17:20
@github-actions
Copy link

github-actions bot commented May 24, 2022 •
edited
Loading

PR #18397: Size comparison from b4586be to 20db28f

Increases (27 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
platform target config section b4586be 20db28f change % change
cc13x2_26x2 all-clusters-app LP_CC2652R7 (read only) 669743 669951 208 0.0
.bss 74836 74900 64 0.1
.text 568864 569072 208 0.0
lock-ftd LP_CC2652R7 (read only) 676471 676679 208 0.0
.bss 72884 72948 64 0.1
.text 581436 581644 208 0.0
lock-mtd LP_CC2652R7 (read only) 625879 626079 200 0.0
(read/write) 145716 145780 64 0.0
.bss 68620 68684 64 0.1
.text 530956 531156 200 0.0
pump-app LP_CC2652R7 (read only) 676331 676539 208 0.0
.bss 73284 73348 64 0.1
.text 587076 587284 208 0.0
pump-controller-app LP_CC2652R7 (read only) 654307 654515 208 0.0
.bss 73140 73204 64 0.1
.text 570280 570488 208 0.0
shell LP_CC2652R7 (read only) 662750 662950 200 0.0
.bss 77196 77260 64 0.1
.text 564900 565100 200 0.0
cyw30739 light cyw930739m2evb_01 (read/write) 624734 625046 312 0.0
.app_xip_area 528012 528260 248 0.0
.bss 79364 79428 64 0.1
lock cyw930739m2evb_01 (read/write) 627402 627722 320 0.1
.app_xip_area 532152 532408 256 0.0
.bss 77924 77988 64 0.1
ota-requestor-no-progress-logging cyw930739m2evb_01 (read/write) 571178 571498 320 0.1
.app_xip_area 466244 466500 256 0.1
.bss 87312 87376 64 0.1
efr32 lighting-app BRD4161A (read only) 916020 916676 656 0.1
(read/write) 133452 133516 64 0.0
.bss 131392 131456 64 0.0
.text 916012 916668 656 0.1
BRD4161A+rpc (read only) 950192 950848 656 0.1
(read/write) 150136 150200 64 0.0
.bss 147872 147936 64 0.0
.text 950184 950840 656 0.1
BRD4161A+rs911x (read only) 790612 791284 672 0.1
(read/write) 129720 129784 64 0.0
.bss 127652 127716 64 0.1
.text 790604 791276 672 0.1
lock-app BRD4161A+wf200 (read only) 947080 947304 224 0.0
(read/write) 124188 124252 64 0.1
.bss 122164 122228 64 0.1
.text 947072 947296 224 0.0
window-app BRD4161A (read only) 897212 897884 672 0.1
(read/write) 133504 133568 64 0.0
.bss 131456 131520 64 0.0
.text 897204 897876 672 0.1
esp32 all-clusters-app c3devkit (read only) 1003030 1003240 210 0.0
(read/write) 1479978 1480042 64 0.0
.dram0.bss 69392 69456 64 0.1
.flash.text 1003030 1003240 210 0.0
m5stack (read only) 1057815 1058107 292 0.0
(read/write) 481992 482056 64 0.0
.dram0.bss 74912 74976 64 0.1
.flash.text 1052431 1052723 292 0.0
k32w light k32w061+release (read/write) 683256 683528 272 0.0
.bss 80432 80496 64 0.1
.text 599104 599312 208 0.0
lock k32w061+release (read/write) 729268 729556 288 0.0
.bss 80856 80920 64 0.1
.text 644732 644956 224 0.0
linux chip-tool-no-interactive-ipv6only arm64 (read only) 9348524 9349676 1152 0.0
.text 7377332 7378484 1152 0.0
thermostat-no-ble arm64 (read only) 2360916 2362036 1120 0.0
(read/write) 177457 177521 64 0.0
.bss 88193 88257 64 0.1
.text 1984032 1985152 1120 0.1
mbed lock-app CY8CPROTO_062_4343W+release (read/write) 2419208 2419464 256 0.0
.bss 202860 202924 64 0.0
.text 1381852 1382108 256 0.0
nrfconnect all-clusters-app nrf52840dk_nrf52840 (read/write) 1184539 1184779 240 0.0
bss 139560 139624 64 0.0
text 812448 812692 244 0.0
p6 all-clusters-app default (read/write) 2541304 2541976 672 0.0
.bss 137360 137424 64 0.0
.text 1499568 1500240 672 0.0
light-app default (read/write) 2424616 2425288 672 0.0
.bss 129696 129760 64 0.0
.text 1382880 1383552 672 0.0
lock-app default (read/write) 2435176 2435832 656 0.0
.bss 129504 129568 64 0.0
.text 1393440 1394096 656 0.0
telink light-switch-app tlsr9518adk80d (read/write) 782900 783172 272 0.0
bss 70828 70892 64 0.1
text 553508 553716 208 0.0
lighting-app tlsr9518adk80d (read/write) 802924 803196 272 0.0
bss 71080 71144 64 0.1
text 570242 570450 208 0.0
Decreases (5 builds for cc13x2_26x2)
platform target config section b4586be 20db28f change % change
cc13x2_26x2 all-clusters-app LP_CC2652R7 (read/write) 182112 181968 -144 -0.1
lock-ftd LP_CC2652R7 (read/write) 166376 166232 -144 -0.1
pump-app LP_CC2652R7 (read/write) 167940 167796 -144 -0.1
pump-controller-app LP_CC2652R7 (read/write) 189564 189420 -144 -0.1
shell LP_CC2652R7 (read/write) 184664 184528 -136 -0.1
Full report (27 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
platform target config section b4586be 20db28f change % change
cc13x2_26x2 all-clusters-app LP_CC2652R7 (read only) 669743 669951 208 0.0
(read/write) 182112 181968 -144 -0.1
.bss 74836 74900 64 0.1
.data 3404 3404 0 0.0
.rodata 100655 100655 0 0.0
.text 568864 569072 208 0.0
lock-ftd LP_CC2652R7 (read only) 676471 676679 208 0.0
(read/write) 166376 166232 -144 -0.1
.bss 72884 72948 64 0.1
.data 3236 3236 0 0.0
.rodata 94551 94551 0 0.0
.text 581436 581644 208 0.0
lock-mtd LP_CC2652R7 (read only) 625879 626079 200 0.0
(read/write) 145716 145780 64 0.0
.bss 68620 68684 64 0.1
.data 3236 3236 0 0.0
.rodata 94431 94431 0 0.0
.text 530956 531156 200 0.0
pump-app LP_CC2652R7 (read only) 676331 676539 208 0.0
(read/write) 167940 167796 -144 -0.1
.bss 73284 73348 64 0.1
.data 3272 3272 0 0.0
.rodata 88771 88771 0 0.0
.text 587076 587284 208 0.0
pump-controller-app LP_CC2652R7 (read only) 654307 654515 208 0.0
(read/write) 189564 189420 -144 -0.1
.bss 73140 73204 64 0.1
.data 3232 3232 0 0.0
.rodata 83547 83547 0 0.0
.text 570280 570488 208 0.0
shell LP_CC2652R7 (read only) 662750 662950 200 0.0
(read/write) 184664 184528 -136 -0.1
.bss 77196 77260 64 0.1
.data 3408 3408 0 0.0
.rodata 97622 97622 0 0.0
.text 564900 565100 200 0.0
cyw30739 light cyw930739m2evb_01 (read/write) 624734 625046 312 0.0
.app_xip_area 528012 528260 248 0.0
.bss 79364 79428 64 0.1
.data 708 708 0 0.0
.rodata 0 0 0 0.0
.text 0 0 0 0.0
lock cyw930739m2evb_01 (read/write) 627402 627722 320 0.1
.app_xip_area 532152 532408 256 0.0
.bss 77924 77988 64 0.1
.data 672 672 0 0.0
.rodata 0 0 0 0.0
.text 0 0 0 0.0
ota-requestor-no-progress-logging cyw930739m2evb_01 (read/write) 571178 571498 320 0.1
.app_xip_area 466244 466500 256 0.1
.bss 87312 87376 64 0.1
.data 584 584 0 0.0
.rodata 0 0 0 0.0
.text 112 112 0 0.0
efr32 lighting-app BRD4161A (read only) 916020 916676 656 0.1
(read/write) 133452 133516 64 0.0
.bss 131392 131456 64 0.0
.data 2060 2060 0 0.0
.text 916012 916668 656 0.1
BRD4161A+rpc (read only) 950192 950848 656 0.1
(read/write) 150136 150200 64 0.0
.bss 147872 147936 64 0.0
.data 2264 2264 0 0.0
.text 950184 950840 656 0.1
BRD4161A+rs911x (read only) 790612 791284 672 0.1
(read/write) 129720 129784 64 0.0
.bss 127652 127716 64 0.1
.data 2068 2068 0 0.0
.text 790604 791276 672 0.1
lock-app BRD4161A+wf200 (read only) 947080 947304 224 0.0
(read/write) 124188 124252 64 0.1
.bss 122164 122228 64 0.1
.data 2024 2024 0 0.0
.text 947072 947296 224 0.0
window-app BRD4161A (read only) 897212 897884 672 0.1
(read/write) 133504 133568 64 0.0
.bss 131456 131520 64 0.0
.data 2048 2048 0 0.0
.text 897204 897876 672 0.1
esp32 all-clusters-app c3devkit (read only) 1003030 1003240 210 0.0
(read/write) 1479978 1480042 64 0.0
.dram0.bss 69392 69456 64 0.1
.dram0.data 14624 14624 0 0.0
.flash.rodata 210536 210536 0 0.0
.flash.text 1003030 1003240 210 0.0
.iram0.text 62954 62954 0 0.0
m5stack (read only) 1057815 1058107 292 0.0
(read/write) 481992 482056 64 0.0
.dram0.bss 74912 74976 64 0.1
.dram0.data 34200 34200 0 0.0
.flash.rodata 240884 240884 0 0.0
.flash.text 1052431 1052723 292 0.0
.iram0.text 123267 123267 0 0.0
k32w light k32w061+release (read/write) 683256 683528 272 0.0
.bss 80432 80496 64 0.1
.data 2016 2016 0 0.0
.text 599104 599312 208 0.0
lock k32w061+release (read/write) 729268 729556 288 0.0
.bss 80856 80920 64 0.1
.data 1976 1976 0 0.0
.text 644732 644956 224 0.0
linux chip-tool-no-interactive-ipv6only arm64 (read only) 9348524 9349676 1152 0.0
(read/write) 663057 663057 0 0.0
.bss 42241 42241 0 0.0
.data 1192 1192 0 0.0
.data.rel.ro 600760 600760 0 0.0
.dynamic 560 560 0 0.0
.got 15024 15024 0 0.0
.init 24 24 0 0.0
.init_array 184 184 0 0.0
.rodata 456748 456748 0 0.0
.text 7377332 7378484 1152 0.0
thermostat-no-ble arm64 (read only) 2360916 2362036 1120 0.0
(read/write) 177457 177521 64 0.0
.bss 88193 88257 64 0.1
.data 1520 1520 0 0.0
.data.rel.ro 79944 79944 0 0.0
.dynamic 560 560 0 0.0
.got 4768 4768 0 0.0
.init 24 24 0 0.0
.init_array 376 376 0 0.0
.rodata 147812 147812 0 0.0
.text 1984032 1985152 1120 0.1
mbed lock-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2419208 2419464 256 0.0
.bss 202860 202924 64 0.0
.data 5872 5872 0 0.0
.text 1381852 1382108 256 0.0
nrfconnect all-clusters-app nrf52840dk_nrf52840 (read/write) 1184539 1184779 240 0.0
bss 139560 139624 64 0.0
rodata 153684 153684 0 0.0
text 812448 812692 244 0.0
p6 all-clusters-app default (read/write) 2541304 2541976 672 0.0
.bss 137360 137424 64 0.0
.data 2808 2808 0 0.0
.text 1499568 1500240 672 0.0
light-app default (read/write) 2424616 2425288 672 0.0
.bss 129696 129760 64 0.0
.data 2608 2608 0 0.0
.text 1382880 1383552 672 0.0
lock-app default (read/write) 2435176 2435832 656 0.0
.bss 129504 129568 64 0.0
.data 2568 2568 0 0.0
.text 1393440 1394096 656 0.0
telink light-switch-app tlsr9518adk80d (read/write) 782900 783172 272 0.0
bss 70828 70892 64 0.1
noinit 40416 40416 0 0.0
text 553508 553716 208 0.0
lighting-app tlsr9518adk80d (read/write) 802924 803196 272 0.0
bss 71080 71144 64 0.1
noinit 40416 40416 0 0.0
text 570242 570450 208 0.0

mrjerryjohns
mrjerryjohns reviewed May 27, 2022
View reviewed changes
src/transport/SessionHolder.h

private:
Optional<ReferenceCountedHandle<Transport::Session>> mSession;
Optional<std::reference_wrapper<Transport::Session>> mSession;
Copy link
Contributor

@mrjerryjohns mrjerryjohns May 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't all of this just become Transport::Session *mSession? What's the value to saying it's an "optional reference"?

@kghost
Copy link
Contributor Author

kghost commented May 31, 2022

#18431 This issue raised by Tennessee makes me feel that is may not feasible to map SessionHandle/SessionHolder to shared_ptr/weak_ptr.

We almost gain nothing from this PR.

@kghost kghost closed this May 31, 2022
@mrjerryjohns
Copy link
Contributor

So, in conversations with @kghost, there is an issue with UnauthenticatedSession that creates a wrinkle here.

Currently, the SDK uses SessionHolder within the ExchangeContext as a form of a strong reference to keep the underlying UnauthenticatedSession alive. This EC is then kept alive by the ReliableMessageMgr::RetransTableEntry while there are messages with pending ACKs. Consequently, even if the PairingSession eventually completes, the EC being kept alive in the retrans table (and consequently, the session) means that the session is kept resident till the final ACK is received.

In the new weak-ref model being espoused here, the EC would now only hold a weak-reference to the session. This goes with the general philosophy that EC's should get terminated when sessions get terminated (and to do that, having the EC hold a weak-ref would seem most natural). However in this scheme, it's now possible for the UnauthenticatedSession to possibly get evicted by LRU logic today when the PairingSession is done its work. This would evict the final message from the retrans table, preventing a recipient from properly receiving that message.

Possible solutions include:

  • Keep the existing mixed strong/weak ref model for SessionHolder that acts differently depending on session type.
  • Alter the LRU to incorporate the # of weak-refs currently present on an UnauthenticatedSession and select ones that have less references.
  • Possible proposal from @kghost to make more explicit the strong vs. weak modality in secure/insecure settings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrjerryjohns mrjerryjohns mrjerryjohns left review comments

@msandstedt msandstedt Awaiting requested review from msandstedt

@anush-apple anush-apple Awaiting requested review from anush-apple

@arkq arkq Awaiting requested review from arkq

@Byungjoo-Lee Byungjoo-Lee Awaiting requested review from Byungjoo-Lee

@bzbarsky-apple bzbarsky-apple Awaiting requested review from bzbarsky-apple

@carol-apple carol-apple Awaiting requested review from carol-apple

@chrisdecenzo chrisdecenzo Awaiting requested review from chrisdecenzo

@chshu chshu Awaiting requested review from chshu

@chulspro chulspro Awaiting requested review from chulspro

@Damian-Nordic Damian-Nordic Awaiting requested review from Damian-Nordic

@dhrishi dhrishi Awaiting requested review from dhrishi

@electrocucaracha electrocucaracha Awaiting requested review from electrocucaracha

@emargolis emargolis Awaiting requested review from emargolis

@franck-apple franck-apple Awaiting requested review from franck-apple

@gjc13 gjc13 Awaiting requested review from gjc13

@harimau-qirex harimau-qirex Awaiting requested review from harimau-qirex

@hawk248 hawk248 Awaiting requested review from hawk248

@harsha-rajendran harsha-rajendran Awaiting requested review from harsha-rajendran

@isiu-apple isiu-apple Awaiting requested review from isiu-apple

@jelderton jelderton Awaiting requested review from jelderton

@jepenven-silabs jepenven-silabs Awaiting requested review from jepenven-silabs

@jmartinez-silabs jmartinez-silabs Awaiting requested review from jmartinez-silabs

@jtung-apple jtung-apple Awaiting requested review from jtung-apple

@lazarkov lazarkov Awaiting requested review from lazarkov

@LuDuda LuDuda Awaiting requested review from LuDuda

@mlepage-google mlepage-google Awaiting requested review from mlepage-google

@mspang mspang Awaiting requested review from mspang

@rgoliver rgoliver Awaiting requested review from rgoliver

@robszewczyk robszewczyk Awaiting requested review from robszewczyk

@sagar-apple sagar-apple Awaiting requested review from sagar-apple

@saurabhst saurabhst Awaiting requested review from saurabhst

@selissia selissia Awaiting requested review from selissia

@tcarmelveilleux tcarmelveilleux Awaiting requested review from tcarmelveilleux

@tecimovic tecimovic Awaiting requested review from tecimovic

@vijs vijs Awaiting requested review from vijs

@vivien-apple vivien-apple Awaiting requested review from vivien-apple

@wbschiller wbschiller Awaiting requested review from wbschiller

@woody-apple woody-apple Awaiting requested review from woody-apple

@xylophone21 xylophone21 Awaiting requested review from xylophone21

@yufengwangca yufengwangca Awaiting requested review from yufengwangca

@yunhanw-google yunhanw-google Awaiting requested review from yunhanw-google

Assignees
No one assigned
Labels
lib review - pending transport
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kghost @mrjerryjohns