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.

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

Reduce dangling session handle #9225

Merged
merged 1 commit into from
Sep 2, 2021

Conversation

kghost
Copy link
Contributor

@kghost kghost commented Aug 25, 2021

Problem

There are lots of dangling session handle in our code, due to following 2 reasons:

  1. The session is out of its life-span
  2. The session is a unauthenticated session (PASE and CASE pairing)

Case.1 is solved by using Optional<SessionHandle>
Case.2 will be solved in the future by assigning an unauthenticated session, associate the session with its peer address

Change overview

  • Remove default constructor of SessionHandle
  • Replace some SessionHandle field with Optional<SessionHandle>, if the life-span of the field can be longer than the session. For example, ExchangeContext::mSessionHandle, the ExchangeContext can still exist even if the session has been closed, in this circumstance, the mSessionHandle should not hold a session.

Testing

Manually tested using unit-tests.

@todo
Copy link

todo bot commented Aug 25, 2021

currently SessionHandle is not able to identify a unauthenticated session, create an empty handle for it

// TODO: currently SessionHandle is not able to identify a unauthenticated session, create an empty handle for it
static SessionHandle TemporaryUnauthenticatedSession()
{
return SessionHandle(kPlaceholderNodeId, Transport::kUndefinedFabricIndex);
}
private:
friend class SecureSessionMgr;
NodeId mPeerNodeId;


This comment was generated by todo based on a TODO comment in 0bfac14 in #9225. cc @kghost.

src/controller/CHIPDevice.h Show resolved Hide resolved
mspang
mspang previously requested changes Aug 27, 2021
Copy link
Contributor

@mspang mspang left a comment

Choose a reason for hiding this comment

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

Marking Request changes while we discuss the optional reimplementation..

@kghost kghost force-pushed the dangling-sessoin-handle branch 3 times, most recently from 149950e to 849d27c Compare August 31, 2021 21:48
@kghost kghost requested a review from mspang August 31, 2021 21:56
@kghost
Copy link
Contributor Author

kghost commented Sep 1, 2021

@mspang I have rebased the PR to use the new optional implementation.

@kghost kghost force-pushed the dangling-sessoin-handle branch 2 times, most recently from 1c37e36 to d835a0c Compare September 1, 2021 05:40
@github-actions
Copy link

github-actions bot commented Sep 2, 2021

Size increase report for "gn_qpg-example-build" from 9dfa537

File Section File VM
chip-qpg6100-lighting-example.out .heap 0 32
chip-qpg6100-lighting-example.out .text -8 -8
chip-qpg6100-lighting-example.out .bss 0 -32
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-qpg6100-lighting-example.out and ./pull_artifact/chip-qpg6100-lighting-example.out:

sections,vmsize,filesize
.heap,32,0
[Unmapped],0,8
.shstrtab,0,3
.text,-8,-8
.strtab,0,-15
.symtab,0,-16
.bss,-32,0
.debug_aranges,0,-376
.debug_frame,0,-1304
.debug_ranges,0,-2624
.debug_abbrev,0,-6045
.debug_str,0,-9663
.debug_loc,0,-13752
.debug_line,0,-16118
.debug_info,0,-132630

Comparing ./master_artifact/chip-qpg6100-lighting-example.out.map and ./pull_artifact/chip-qpg6100-lighting-example.out.map:

BLOAT EXECUTION FAILED WITH CODE 1:
bloaty: unknown file type for file './pull_artifact/chip-qpg6100-lighting-example.out.map'


@github-actions
Copy link

github-actions bot commented Sep 2, 2021

Size increase report for "esp32-example-build" from 9dfa537

File Section File VM
chip-bridge-app.elf .dram0.bss 0 -32
chip-bridge-app.elf .flash.text -84 -84
chip-temperature-measurement-app.elf .dram0.bss 0 -32
chip-temperature-measurement-app.elf .flash.text -100 -100
chip-all-clusters-app.elf .dram0.bss 0 -32
chip-all-clusters-app.elf .flash.text -32 -32
chip-lock-app.elf .flash.text -16 -16
chip-lock-app.elf .dram0.bss 0 -32
chip-shell.elf .dram0.bss 0 72
chip-shell.elf .flash.text -12 -12
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-ipv6only-app.elf and ./pull_artifact/chip-ipv6only-app.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-bridge-app.elf and ./pull_artifact/chip-bridge-app.elf:

sections,vmsize,filesize
.debug_info,0,32815
.debug_str,0,1107
.debug_loc,0,109
[Unmapped],0,84
.debug_frame,0,24
.debug_aranges,0,8
.debug_abbrev,0,-12
.dram0.bss,-32,0
.flash.text,-84,-84
.debug_ranges,0,-288
.debug_line,0,-491

Comparing ./master_artifact/chip-temperature-measurement-app.elf and ./pull_artifact/chip-temperature-measurement-app.elf:

sections,vmsize,filesize
.debug_info,0,29451
.debug_str,0,1106
[Unmapped],0,100
.debug_loc,0,89
.debug_frame,0,24
.debug_aranges,0,8
.debug_abbrev,0,-12
.dram0.bss,-32,0
.flash.text,-100,-100
.debug_ranges,0,-256
.debug_line,0,-482

Comparing ./master_artifact/chip-pigweed-app.elf and ./pull_artifact/chip-pigweed-app.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-all-clusters-app.elf and ./pull_artifact/chip-all-clusters-app.elf:

sections,vmsize,filesize
.debug_info,0,48810
.debug_str,0,1107
.debug_loc,0,164
.debug_frame,0,40
[Unmapped],0,32
.debug_abbrev,0,18
.debug_aranges,0,8
.riscv.attributes,0,-2
.dram0.bss,-32,0
.flash.text,-32,-32
.debug_ranges,0,-160
.debug_line,0,-237

Comparing ./master_artifact/chip-lock-app.elf and ./pull_artifact/chip-lock-app.elf:

sections,vmsize,filesize
.debug_info,0,33571
.debug_str,0,1107
.debug_loc,0,265
.debug_frame,0,24
[Unmapped],0,16
.debug_abbrev,0,12
.debug_aranges,0,8
.flash.text,-16,-16
.dram0.bss,-32,0
.debug_ranges,0,-288
.debug_line,0,-491

Comparing ./master_artifact/chip-persistent-storage.elf and ./pull_artifact/chip-persistent-storage.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize
.debug_info,0,5837
.debug_str,0,1017
.dram0.bss,72,0
.debug_frame,0,24
[Unmapped],0,12
.debug_aranges,0,8
.debug_abbrev,0,-2
.flash.text,-12,-12
.debug_loc,0,-187
.debug_ranges,0,-232
.debug_line,0,-321


@github-actions
Copy link

github-actions bot commented Sep 2, 2021

Size increase report for "nrfconnect-example-build" from 9dfa537

File Section File VM
chip-lock.elf device_handles 4 4
chip-lock.elf bss 0 -32
chip-lock.elf text -100 -100
chip-shell.elf bss 0 81
chip-shell.elf [LOAD #3 [RW]] 0 -17
chip-shell.elf text -112 -112
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-lock.elf and ./pull_artifact/chip-lock.elf:

sections,vmsize,filesize
.debug_info,0,23994
.debug_str,0,1107
.debug_loc,0,201
.debug_frame,0,72
.debug_aranges,0,24
.debug_abbrev,0,15
device_handles,4,4
bss,-32,0
text,-100,-100
.debug_ranges,0,-144
.debug_line,0,-197

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize
.debug_info,0,6304
.debug_str,0,976
bss,81,0
.debug_frame,0,28
.debug_aranges,0,16
.debug_abbrev,0,-6
[LOAD #3 [RW]],-17,0
text,-112,-112
.debug_loc,0,-144
.debug_ranges,0,-152
.debug_line,0,-270


@mspang mspang merged commit 50feebe into project-chip:master Sep 2, 2021
mkardous-silabs pushed a commit to mkardous-silabs/connectedhomeip that referenced this pull request Sep 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants