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

Fix application (chip-device-ctrl) hang caused by mdns resolve callback not being called #7229

Closed
wants to merge 0 commits into from

Conversation

bluebin14
Copy link
Contributor

@bluebin14 bluebin14 commented May 28, 2021

Problem

What is being fixed?

chip-device-ctrl always hangs when issuing resolve command. This happens in both cases, on resolve success and failure.

Change overview

What's in this PR

DeviceCommissioner::OnNodeIdResolved and DeviceCommissioner::OnNodeIdResolutionFailed fails to forward resolve results to DeviceController routines with same name since at that point the pairing and credentials processing is already finished when the variable mDeviceBeingPaired is reset to kNumMaxActiveDevices in DeviceCommissioner::RendezvousCleanup. This causes hang of the application which waits forever for a callback that never comes and has to be force quit.

Testing

How was this tested? (at least one bullet point required)

  • launch chip-device-ctrl
  • commission to thread network, e.g.

connect -ble 3840 12345678 12344321
zcl NetworkCommissioning AddThreadNetwork 12344321 0 0 operationalDataset=hex:000300001902088836262BBA63449605102799757C265665FC001078FDE764698D01022DF2 breadcrumb=0 timeoutMs=5000
zcl NetworkCommissioning EnableNetwork 12344321 0 0 networkID=hex:8836262BBA634496 breadcrumb=0 timeoutMs=5000
close-ble

  • issue resolve command, e.g.

resolve 5544332211 12344321

  • result without the fix: application hangs and needs to be force quit

  • result with the fix: application does not hang, device address gets correctly updated. Sample output:

chip-device-ctrl > resolve 5544332211 12344321
[1622196594583] [0xd702a] CHIP: [DL] Mdns: OnGetAddrInfo hostname:chiplock.local.
[1622196607629] [0xd702a] CHIP: [DIS] Node ID resolved for 0x0000000000BC5C01
[1622196778078] [0xd702a] CHIP: [CTL] SyncSetKeyValue on PairedDevicebc5c01
Node address has been updated
Current address: fdd4:5b3d:373d::60db:1ad2:1bdc:846d:11097
chip-device-ctrl >

Notes

The first part DeviceCommissioner::OnNodeIdResolved is now identical to master due to subsequent #7213 being merged, but the second part DeviceCommissioner::OnNodeIdResolutionFailed is not addressed by 7213 and needs the same fix as here.

Resolved editorial conflict with master but my initial version was better, there is no point in declaring Device * device = nullptr; outside of if block.. other than that it is the same.

Copy link
Contributor

@bzbarsky-apple bzbarsky-apple left a comment

Choose a reason for hiding this comment

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

Please rebase on top of master so it's clear what's actually being changed.

And it seems like we could still move the decl of device into the if block in OnNodeIdResolved...

@bzbarsky-apple
Copy link
Contributor

It looks like #7213 missed the error case, basically?

@bluebin14 bluebin14 closed this May 29, 2021
@bluebin14 bluebin14 deleted the mdns-hang-fix branch May 29, 2021 10:31
@bluebin14
Copy link
Contributor Author

Rebase error, not bothering to reopen..

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