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

Remove the OnSessionReleased callback from OperationalSessionSetup. #26395

Conversation

bzbarsky-apple
Copy link
Contributor

Since OperationalSessionSetup is ephemeral, it only holds on to a session while it's notifying its listeners, after which it will delete itself.

Right now it was deleting itself from OnSessionReleased, but that means it could end up with a double-delete... and also, it's already notifying listeners if it has a session, so there is no point, or ability, to notify them again on session release.

The changes here:

  1. Take out the OnSessionReleased that couldn't do anything except lead to use-after-free.
  2. Fix a bug on OnSessionEstablished where if we got a session that's not usable we leaked and left our listeners hanging instead of just notifying our listeners with error.

Since OperationalSessionSetup is ephemeral, it only holds on to a session while
it's notifying its listeners, after which it will delete itself.

Right now it was deleting itself from OnSessionReleased, but that means it could
end up with a double-delete... and also, it's already notifying listeners if it
has a session, so there is no point, or ability, to notify them again on session
release.

The changes here:

1. Take out the OnSessionReleased that couldn't do anything except lead to
   use-after-free.
2. Fix a bug on OnSessionEstablished where if we got a session that's not usable
   we leaked and left our listeners hanging instead of just notifying our
   listeners with error.
@github-actions
Copy link

github-actions bot commented May 5, 2023

PR #26395: Size comparison from ed8d546 to e3fe5c7

Increases (1 build for cc32xx)
platform target config section ed8d546 e3fe5c7a change % change
cc32xx lock CC3235SF_LAUNCHXL .debug_abbrev 957621 957646 25 0.0
Decreases (1 build for cc32xx)
platform target config section ed8d546 e3fe5c7a change % change
cc32xx lock CC3235SF_LAUNCHXL (read only) 605066 604946 -120 -0.0
.debug_aranges 101136 101040 -96 -0.1
.debug_frame 341512 341260 -252 -0.1
.debug_info 19609713 19608209 -1504 -0.0
.debug_line 2666606 2666198 -408 -0.0
.debug_loclists 1489165 1488858 -307 -0.0
.debug_rnglists 94315 94243 -72 -0.1
.debug_str 3108961 3108654 -307 -0.0
.rodata 104362 104330 -32 -0.0
.strtab 483384 483077 -307 -0.1
.symtab 287328 287168 -160 -0.1
.text 498580 498492 -88 -0.0
Full report (1 build for cc32xx)
platform target config section ed8d546 e3fe5c7a change % change
cc32xx lock CC3235SF_LAUNCHXL 0 0 0 0.0
(read only) 605066 604946 -120 -0.0
(read/write) 204164 204164 0 0.0
.ARM.attributes 44 44 0 0.0
.ARM.exidx 8 8 0 0.0
.bss 197576 197576 0 0.0
.comment 206 206 0 0.0
.data 1468 1468 0 0.0
.debug_abbrev 957621 957646 25 0.0
.debug_aranges 101136 101040 -96 -0.1
.debug_frame 341512 341260 -252 -0.1
.debug_info 19609713 19608209 -1504 -0.0
.debug_line 2666606 2666198 -408 -0.0
.debug_line_str 513 513 0 0.0
.debug_loc 33340 33340 0 0.0
.debug_loclists 1489165 1488858 -307 -0.0
.debug_ranges 4984 4984 0 0.0
.debug_rnglists 94315 94243 -72 -0.1
.debug_str 3108961 3108654 -307 -0.0
.ramVecs 780 780 0 0.0
.resetVecs 64 64 0 0.0
.rodata 104362 104330 -32 -0.0
.shstrtab 265 265 0 0.0
.stack 2048 2048 0 0.0
.strtab 483384 483077 -307 -0.1
.symtab 287328 287168 -160 -0.1
.text 498580 498492 -88 -0.0

@andy31415 andy31415 merged commit 001cdab into project-chip:master May 8, 2023
@bzbarsky-apple bzbarsky-apple deleted the fix-session-setup-session-released branch May 8, 2023 16:14
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.

3 participants