Skip to content

Thread Safety Issue in Heartbeat Timer Implementation #23

@XisGender

Description

@XisGender

There is a thread safety issue in the heartbeat timer implementation in both TJPNetworkManagerV1 and TJPConcurrentNetworkManager. The issue arises from accessing the lastHeartbeatTime property from multiple threads without proper synchronization.

Steps to Reproduce

  1. Create a connection that experiences intermittent network issues
  2. Observe the heartbeat timer behavior under high network load or when device resources are constrained
  3. App may crash or behave unexpectedly

Current Behavior

In the heartbeat timer event handler, lastHeartbeatTime is accessed directly with self->_lastHeartbeatTime without proper thread synchronization. The timer callback runs on the network queue, but the property might be accessed from other threads, leading to potential race conditions.

Expected Behavior

Access to shared mutable state like lastHeartbeatTime should be properly synchronized across all threads to prevent race conditions.

Code References

// In TJPConcurrentNetworkManager.m:
dispatch_source_set_event_handler(self.heartbeatTimer, ^{
    // Updates lastHeartbeatTime
    weakSelf.lastHeartbeatTime = [NSDate date];
    
    // Directly accesses the instance variable from a different thread context
    if ([[NSDate date] timeIntervalSinceDate:self->_lastHeartbeatTime] > 30) {
        TJPLOG_WARN(@"心跳超时,主动断开连接");
        [weakSelf disconnectAndRetry];
    }
});

Impact

  • Race conditions can lead to:
    • Incorrect heartbeat timeout detection
    • Premature connection terminations
    • Unnecessary reconnection attempts
    • Crashes due to memory access violations
  • Difficult-to-reproduce bugs that only occur under specific timing conditions

Proposed Solution

  1. Use proper thread synchronization when accessing shared state:
- (void)startHeartbeat {
    [self stopHeartbeat];
    
    __weak typeof(self) weakSelf = self;
    self.heartbeatTimer = dispatch_source_create(DISPATCH_SOURCE_TYPE_TIMER, 0, 0, self->_networkQueue);
    
    dispatch_source_set_timer(self.heartbeatTimer, dispatch_time(DISPATCH_TIME_NOW, 15 * NSEC_PER_SEC), 15 * NSEC_PER_SEC, 1 * NSEC_PER_SEC);
    
    dispatch_source_set_event_handler(self.heartbeatTimer, ^{
        // Create strong reference to avoid deallocation during block execution
        __strong typeof(weakSelf) strongSelf = weakSelf;
        if (!strongSelf) return;
        
        // Send heartbeat
        [strongSelf sendHeartbeat];
        
        // Update timestamp safely on the network queue
        dispatch_async(strongSelf->_networkQueue, ^{
            strongSelf.lastHeartbeatTime = [NSDate date];
            
            // Check timeout using the synchronized property access
            NSTimeInterval timeSinceLastHeartbeat = [[NSDate date] timeIntervalSinceDate:strongSelf.lastHeartbeatTime];
            if (timeSinceLastHeartbeat > 30) {
                TJPLOG_WARN(@"心跳超时,主动断开连接");
                [strongSelf disconnectAndRetry];
            }
        });
    });
    
    dispatch_resume(self.heartbeatTimer);
}
  1. Alternatively, implement proper accessor methods with synchronization:
- (void)setLastHeartbeatTime:(NSDate *)date {
    dispatch_async(self->_networkQueue, ^{
        self->_lastHeartbeatTime = date;
    });
}

- (NSDate *)lastHeartbeatTime {
    __block NSDate *result;
    dispatch_sync(self->_networkQueue, ^{
        result = self->_lastHeartbeatTime;
    });
    return result;
}

These changes would ensure that all access to shared state happens in a thread-safe manner, preventing race conditions and related crashes.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions