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

Don't automatically close exchanges if the underlying connection closes. #7114

Merged

Conversation

bzbarsky-apple
Copy link
Contributor

Someone might still have a reference to those exchanges, and we will
end up with an unbalanced release. Instead just notify our consumer
in the specific case when we are waiting for a response so it knows
the response will never come (and presumably closes the exchange as
needed).

Problem

Fixes #7012

Change overview

Instead of hard-closing exchanges on connection close, just notify the ones that are waiting for a response that it won't be coming.

Testing

src/messaging/ExchangeMgr.h Show resolved Hide resolved
src/messaging/ExchangeContext.cpp Outdated Show resolved Hide resolved
@github-actions
Copy link

Size increase report for "esp32-example-build" from 06d606f

File Section File VM
chip-all-clusters-app.elf .flash.text 92 92
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

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

sections,vmsize,filesize
.debug_info,0,3501
.debug_line,0,256
.debug_str,0,142
.strtab,0,120
.flash.text,92,92
.debug_loc,0,63
.debug_frame,0,48
.debug_abbrev,0,32
.symtab,0,32
.debug_aranges,0,16
.debug_ranges,0,16
.xt.prop._ZTVN4chip11DeviceLayer37DeviceNetworkProvisioningDelegateImplE,0,2

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

sections,vmsize,filesize


@github-actions
Copy link

Size increase report for "gn_qpg6100-example-build" from 06d606f

File Section File VM
chip-qpg6100-lighting-example.out .text 96 96
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

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'

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

sections,vmsize,filesize
.debug_info,0,2762
.debug_loc,0,456
.debug_line,0,181
.debug_str,0,141
.strtab,0,120
.text,96,96
.symtab,0,64
.debug_frame,0,48
.debug_ranges,0,40
.debug_aranges,0,16
[Unmapped],0,-96


@github-actions
Copy link

Size increase report for "nrfconnect-example-build" from 06d606f

File Section File VM
chip-lock.elf text 88 88
chip-lock.elf device_handles 8 8
chip-shell.elf text 88 88
chip-shell.elf device_handles -8 -8
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,2814
.debug_loc,0,382
.debug_line,0,182
.debug_str,0,142
.strtab,0,120
text,88,88
.symtab,0,64
.debug_frame,0,48
.debug_ranges,0,40
.debug_aranges,0,16
device_handles,8,8

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

sections,vmsize,filesize
.debug_info,0,942
.debug_loc,0,458
.debug_line,0,178
.debug_str,0,142
.strtab,0,120
text,88,88
.symtab,0,64
.debug_frame,0,48
.debug_ranges,0,40
.debug_aranges,0,16
device_handles,-8,-8


// connection state) value, because it's still referencing the now-expired
// connection. This will mean that no more messages can be sent via this
// exchange, which seems fine given the semantics of connection expiration.
mSecureSession = SecureSessionHandle();
Copy link
Contributor

@yufengwangca yufengwangca May 26, 2021

Choose a reason for hiding this comment

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

After we reset the SessionHandle, this exchange can not be used to send any message.

Let's go back the the original issue.

After we send AddRootCert command and receive the response to AddRootCert, the connection is closed.
Now we don't close the exchange, but reset the SessionHandle of the exchange.

On which exchange we send AddNetwork Command? My understand is we should continue to use the same exchange to send this command and close the exchange when we reach the end of conversation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On which exchange we send AddNetwork Command?

A different one. Each new Invoke is a new exchange in the spec.

Someone might still have a reference to those exchanges, and we will
end up with an unbalanced release.  Instead just notify our consumer
in the specific case when we are waiting for a response so it knows
the response will never come (and presumably closes the exchange as
needed).
@bzbarsky-apple bzbarsky-apple force-pushed the dont-close-exchanges-randomly branch from dbef181 to ad3dfa8 Compare May 26, 2021 21:38
@mspang mspang merged commit 7f841f0 into project-chip:master May 26, 2021
@bzbarsky-apple bzbarsky-apple deleted the dont-close-exchanges-randomly branch May 27, 2021 00:22
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
…es. (project-chip#7114)

Someone might still have a reference to those exchanges, and we will
end up with an unbalanced release.  Instead just notify our consumer
in the specific case when we are waiting for a response so it knows
the response will never come (and presumably closes the exchange as
needed).
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.

ExchangeManager::OnConnectionExpired is incorrectly closing exchanges, can cause lost messages or crashes
6 participants