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(IoT): fixing race condition in AWSIoTStreamThread .cxx_destruct … #5469

Closed
wants to merge 3 commits into from

Conversation

AndrKonovalov
Copy link

Issue #, if available:
#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.
  1. 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.
  1. 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.
  1. 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.
  1. 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.
    Check points:

  • [ x] 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.

…ws-amplify#5452

Related issue:
aws-amplify#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.
@AndrKonovalov AndrKonovalov requested review from awsmobilesdk and a team as code owners December 4, 2024 00:16
@AndrKonovalov AndrKonovalov marked this pull request as draft December 4, 2024 00:16
@AndrKonovalov AndrKonovalov marked this pull request as ready for review December 4, 2024 00:16
@harsh62
Copy link
Member

harsh62 commented Dec 9, 2024

Failing unit tests:

Failing tests:
	-[AWSIoTStreamThreadTests testCancel_shouldNotCloseStreams_andInvokeOnStop]
	-[AWSIoTStreamThreadTests testCancelAndDisconnect_shouldCloseStreams_andInvokeOnStop]
	-[AWSIoTStreamThreadTests testCancelAndDisconnect_shouldSetIsCleaningUp]
	-[AWSIoTStreamThreadTests testCancelAndDisconnect_shouldSynchronizeOnCleanupQueue]
	-[AWSIoTStreamThreadTests testInvalidateTimer_shouldInvalidateAndSetTimerToNil]
	-[AWSIoTStreamThreadTests testRunLoop_shouldInvokeRunModeBeforeDate]
	-[AWSIoTStreamThreadTests testStart_shouldOpenStream_andInvokeConnectOnSession]

Copy link
Member

@harsh62 harsh62 left a comment

Choose a reason for hiding this comment

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

Tests need to be fixed.

@ruisebas
Copy link
Member

Closing in favour of #5477

@ruisebas ruisebas closed this Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants