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 System vs Inet and BLE shutdown order #7429

Merged
merged 1 commit into from
Jun 8, 2021

Conversation

kpschoedel
Copy link
Contributor

Problem

Inet and BLE contain pointers to the System::Layer, but in some
implementations System::Layer is shut down first, leaving those
pointers invalid. (There appear to be no current uses of those
invalid pointers in the tree.)

Change overview

Shut down system layer after the others in:

  • DeviceController::Shutdown()
  • GenericPlatformManagerImpl::_Shutdown()
  • JNI_OnUnload()

Testing

  • Some existing unit tests use the correct order (System last),
    so it's known to work.
  • Manually tested with platforms using GenericPlatformManagerImpl::_Shutdown()
    (Linux paired with ESP32).

#### Problem

Inet and BLE contain pointers to the System::Layer, but in some
implementations System::Layer is shut down first, leaving those
pointers invalid. (There appear to be no current uses of those
invalid pointers in the tree.)

#### Change overview

Shut down system layer after the others in:
- `DeviceController::Shutdown()`
- `GenericPlatformManagerImpl::_Shutdown()`
- `JNI_OnUnload()`

#### Testing

- Some existing unit tests use the correct order (System last),
  so it's known to work.
- Manually tested with platforms using `GenericPlatformManagerImpl::_Shutdown()`
  (Linux paired with ESP32).
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.

Looks good, but ideally we would have this ordering dependecy documented on the relevant Shutdown methods....

@andy31415 andy31415 merged commit c5c669e into project-chip:master Jun 8, 2021
@kpschoedel
Copy link
Contributor Author

Looks good, but ideally we would have this ordering dependecy documented on the relevant Shutdown methods....

I will add brief comments for these in the event-loop PR that sparked this change.

@kpschoedel kpschoedel deleted the shutdown-order branch June 8, 2021 15:04
andy31415 pushed a commit that referenced this pull request Jun 9, 2021
#### Problem

Inet and BLE contain pointers to the System::Layer, but in some
implementations System::Layer is shut down first, leaving those
pointers invalid. (There appear to be no current uses of those
invalid pointers in the tree.)

#### Change overview

Shut down system layer after the others in:
- `DeviceController::Shutdown()`
- `GenericPlatformManagerImpl::_Shutdown()`
- `JNI_OnUnload()`

#### Testing

- Some existing unit tests use the correct order (System last),
  so it's known to work.
- Manually tested with platforms using `GenericPlatformManagerImpl::_Shutdown()`
  (Linux paired with ESP32).
doublemis1 pushed a commit to doublemis1/connectedhomeip that referenced this pull request Jul 7, 2021
#### Problem

Inet and BLE contain pointers to the System::Layer, but in some
implementations System::Layer is shut down first, leaving those
pointers invalid. (There appear to be no current uses of those
invalid pointers in the tree.)

#### Change overview

Shut down system layer after the others in:
- `DeviceController::Shutdown()`
- `GenericPlatformManagerImpl::_Shutdown()`
- `JNI_OnUnload()`

#### Testing

- Some existing unit tests use the correct order (System last),
  so it's known to work.
- Manually tested with platforms using `GenericPlatformManagerImpl::_Shutdown()`
  (Linux paired with ESP32).
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
#### Problem

Inet and BLE contain pointers to the System::Layer, but in some
implementations System::Layer is shut down first, leaving those
pointers invalid. (There appear to be no current uses of those
invalid pointers in the tree.)

#### Change overview

Shut down system layer after the others in:
- `DeviceController::Shutdown()`
- `GenericPlatformManagerImpl::_Shutdown()`
- `JNI_OnUnload()`

#### Testing

- Some existing unit tests use the correct order (System last),
  so it's known to work.
- Manually tested with platforms using `GenericPlatformManagerImpl::_Shutdown()`
  (Linux paired with ESP32).
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.

5 participants