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.

Sign up for GitHub

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

Jump to bottom

fix(IoT): Fixing race conditions during cleanup in AWSIoTStreamThread #5477

Merged
merged 3 commits into from
Dec 16, 2024
Merged

fix(IoT): Fixing race conditions during cleanup in AWSIoTStreamThread #5477

merged 3 commits into from
Dec 16, 2024

Conversation

ruisebas
Copy link
Member

Issue #, if available:

Description of changes:

This PR works on top of #5469 to address the issues that caused the unit tests to crash/fail.

The overall changes include:

  • Adding a serial dispatch queue to synchronize the reading and writing of internal properties
  • Fixing a retain cycle with the NSTimer
  • Adding validations to prevent multiple starts or cleanups

Check points:

  • Added new tests to cover change, if needed
  • All unit tests pass
  • All integration tests pass
  • Updated CHANGELOG.md
  • Documentation update for the change if required
  • PR title conforms to conventional commit style

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ruisebas
fix(IoT): fixing race condition in AWSIoTStreamThread .cxx_destruct #…
9a39679 
…5452

Related issue:
#5452

Description of changes:

1. Addition of Synchronization Primitives
New Properties:
 - dispatch_queue_t cleanupQueue
 - dispatch_semaphore_t cleanupSemaphore
 - BOOL isCleaningUp
Purpose: Ensures thread-safe access and modification of critical properties like isRunning, shouldDisconnect, and defaultRunLoopTimer.
Synchronization prevents race conditions during cleanup and cancellation processes.

2. Enhanced shouldContinueRunning Method

Before: Used direct property access without synchronization
After: Introduced synchronization using dispatch_sync for thread-safe checks
Purpose:Prevents inconsistencies if multiple threads attempt to read/write properties simultaneously.

3. Cleanup Enhancements

performCleanup and cleanupResources:
Added explicit synchronization: dispatch_sync and dispatch_semaphore ensure cleanup operations are thread-safe and do not overlap if called multiple times.
Handles complex cleanup sequences safely, such as invalidating timers, disconnecting streams, and deallocating the session.
Purpose: Ensures that cleanup actions (e.g., closing streams and invalidating timers) are thread-safe and only executed once.

4. Timer Initialization
Weak Reference to Prevent Retain Cycles: The timer in setupRunLoop now uses a __weak reference to avoid retain cycles
Before: Used a strong reference (target:self), which could result in a retain cycle.
Purpose: Avoids potential memory leaks by ensuring the thread does not retain itself via the timer.

5. Improved cancel Method
Before: Simple isRunning flag and direct super cancel call
After: Introduced thread-safe handling and ensured timer invalidation
Purpose: Prevents race conditions when canceling the thread, ensuring timers are invalidated and properties are safely updated.
@ruisebas ruisebas force-pushed the ruisebas/streamthread-safe branch from 3077ea4 to 580e1e6 Compare December 13, 2024 20:43
@ruisebas ruisebas merged commit 06ab635 into main Dec 16, 2024
67 checks passed
@ruisebas ruisebas deleted the ruisebas/streamthread-safe branch December 16, 2024 20:31
@ruisebas ruisebas mentioned this pull request Dec 18, 2024
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thisisabhash thisisabhash thisisabhash approved these changes

@awsmobilesdk awsmobilesdk Awaiting requested review from awsmobilesdk awsmobilesdk is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ruisebas @thisisabhash