Refactor socket handling in forwarding handlers to use centralized utilities and remove deprecated functions

2025-06-01 15:35:45 +00:00 · 2025-06-01 15:35:45 +00:00 · 47508eb1eb
commit 47508eb1eb
parent fb147148ef
4 changed files with 124 additions and 113 deletions
--- a/readme.hints.md
+++ b/readme.hints.md
@ -641,3 +641,24 @@ export function setupBidirectionalForwarding(

 ### Migration Notes
 No configuration changes needed. The fix applies to all proxy chains automatically.
+
+## Socket Cleanup Handler Deprecation (v19.5.15+)
+
+### Issue
+The deprecated `createSocketCleanupHandler()` function was still being used in forwarding handlers, despite being marked as deprecated.
+
+### Solution
+Updated all forwarding handlers to use the new centralized socket utilities:
+1. **Replaced `createSocketCleanupHandler()`** with `setupBidirectionalForwarding()` in:
+   - `https-terminate-to-https-handler.ts`
+   - `https-terminate-to-http-handler.ts`
+2. **Removed deprecated function** from `socket-utils.ts`
+
+### Benefits
+- Consistent socket handling across all handlers
+- Proper cleanup in proxy chains (no half-open connections by default)
+- Better backpressure handling with the centralized implementation
+- Reduced code duplication
+
+### Migration Notes
+No user-facing changes. All forwarding handlers now use the same robust socket handling as the main SmartProxy connection handler.
--- a/ts/core/utils/socket-utils.ts
+++ b/ts/core/utils/socket-utils.ts
@ -67,37 +67,6 @@ export function cleanupSocket(
  });
 }

-/**
- * Create a cleanup handler for paired sockets (client and server)
- * @param clientSocket The client socket
- * @param serverSocket The server socket (optional)
- * @param onCleanup Optional callback when cleanup is done
- * @returns A cleanup function that can be called multiple times safely
- * @deprecated Use createIndependentSocketHandlers for better half-open support
- */
-export function createSocketCleanupHandler(
-  clientSocket: plugins.net.Socket | plugins.tls.TLSSocket,
-  serverSocket?: plugins.net.Socket | plugins.tls.TLSSocket | null,
-  onCleanup?: (reason: string) => void
-): (reason: string) => void {
-  let cleanedUp = false;
-  
-  return (reason: string) => {
-    if (cleanedUp) return;
-    cleanedUp = true;
-    
-    // Cleanup both sockets (old behavior - too aggressive)
-    cleanupSocket(clientSocket, 'client', { immediate: true });
-    if (serverSocket) {
-      cleanupSocket(serverSocket, 'server', { immediate: true });
-    }
-    
-    // Call cleanup callback if provided
-    if (onCleanup) {
-      onCleanup(reason);
-    }
-  };
-}

 /**
 * Create independent cleanup handlers for paired sockets that support half-open connections
@ -278,19 +247,6 @@ export function setupBidirectionalForwarding(
  return { cleanupClient, cleanupServer };
 }

-/**
- * Pipe two sockets together with proper cleanup on either end
- * @param socket1 First socket
- * @param socket2 Second socket
- */
-export function pipeSockets(
-  socket1: plugins.net.Socket | plugins.tls.TLSSocket,
-  socket2: plugins.net.Socket | plugins.tls.TLSSocket
-): void {
-  socket1.pipe(socket2);
-  socket2.pipe(socket1);
-}
-
 /**
 * Create a socket with immediate error handling to prevent crashes
 * @param options Socket creation options
--- a/ts/forwarding/handlers/https-terminate-to-http-handler.ts
+++ b/ts/forwarding/handlers/https-terminate-to-http-handler.ts
@ -2,7 +2,7 @@ import * as plugins from '../../plugins.js';
 import { ForwardingHandler } from './base-handler.js';
 import type { IForwardConfig } from '../config/forwarding-types.js';
 import { ForwardingHandlerEvents } from '../config/forwarding-types.js';
-import { createSocketCleanupHandler, setupSocketHandlers, createSocketWithErrorHandler } from '../../core/utils/socket-utils.js';
+import { setupSocketHandlers, createSocketWithErrorHandler, setupBidirectionalForwarding } from '../../core/utils/socket-utils.js';

 /**
 * Handler for HTTPS termination with HTTP backend
@ -100,19 +100,30 @@ export class HttpsTerminateToHttpHandler extends ForwardingHandler {
    let backendSocket: plugins.net.Socket | null = null;
    let dataBuffer = Buffer.alloc(0);
    let connectionEstablished = false;
+    let forwardingSetup = false;
    
-    // Create cleanup handler for all sockets
-    const handleClose = createSocketCleanupHandler(tlsSocket, backendSocket, (reason) => {
-      this.emit(ForwardingHandlerEvents.DISCONNECTED, {
-        remoteAddress,
-        reason
-      });
-      dataBuffer = Buffer.alloc(0);
-      connectionEstablished = false;
-    });
+    // Set up initial error handling for TLS socket
+    const tlsCleanupHandler = (reason: string) => {
+      if (!forwardingSetup) {
+        // If forwarding not set up yet, emit disconnected and cleanup
+        this.emit(ForwardingHandlerEvents.DISCONNECTED, {
+          remoteAddress,
+          reason
+        });
+        dataBuffer = Buffer.alloc(0);
+        connectionEstablished = false;
        
-    // Set up error handling with our cleanup utility
-    setupSocketHandlers(tlsSocket, handleClose, undefined, 'tls');
+        if (!tlsSocket.destroyed) {
+          tlsSocket.destroy();
+        }
+        if (backendSocket && !backendSocket.destroyed) {
+          backendSocket.destroy();
+        }
+      }
+      // If forwarding is setup, setupBidirectionalForwarding will handle cleanup
+    };
+    
+    setupSocketHandlers(tlsSocket, tlsCleanupHandler, undefined, 'tls');
    
    // Set timeout
    const timeout = this.getTimeout();
@ -123,7 +134,7 @@ export class HttpsTerminateToHttpHandler extends ForwardingHandler {
        remoteAddress,
        error: 'TLS connection timeout'
      });
-      handleClose('timeout');
+      tlsCleanupHandler('timeout');
    });
    
    // Handle TLS data
@ -172,30 +183,33 @@ export class HttpsTerminateToHttpHandler extends ForwardingHandler {
              dataBuffer = Buffer.alloc(0);
            }
            
-            // Set up bidirectional data flow
-            tlsSocket.pipe(backendSocket!);
-            backendSocket!.pipe(tlsSocket);
+            // Now set up bidirectional forwarding with proper cleanup
+            forwardingSetup = true;
+            setupBidirectionalForwarding(tlsSocket, backendSocket!, {
+              onCleanup: (reason) => {
+                this.emit(ForwardingHandlerEvents.DISCONNECTED, {
+                  remoteAddress,
+                  reason
+                });
+                dataBuffer = Buffer.alloc(0);
+                connectionEstablished = false;
+                forwardingSetup = false;
+              },
+              enableHalfOpen: false // Close both when one closes
+            });
          }
        });
        
-        // Update the cleanup handler with the backend socket
-        const newHandleClose = createSocketCleanupHandler(tlsSocket, backendSocket, (reason) => {
-          this.emit(ForwardingHandlerEvents.DISCONNECTED, {
-            remoteAddress,
-            reason
-          });
-          dataBuffer = Buffer.alloc(0);
-          connectionEstablished = false;
-        });
-        
-        // Set up handlers for backend socket
-        setupSocketHandlers(backendSocket, newHandleClose, undefined, 'backend');
-        
+        // Additional error logging for backend socket
        backendSocket.on('error', (error) => {
-          this.emit(ForwardingHandlerEvents.ERROR, {
-            remoteAddress,
-            error: `Target connection error: ${error.message}`
-          });
+          if (!connectionEstablished) {
+            // Connection failed during setup
+            this.emit(ForwardingHandlerEvents.ERROR, {
+              remoteAddress,
+              error: `Target connection error: ${error.message}`
+            });
+          }
+          // If connected, setupBidirectionalForwarding handles cleanup
        });
      }
    });
--- a/ts/forwarding/handlers/https-terminate-to-https-handler.ts
+++ b/ts/forwarding/handlers/https-terminate-to-https-handler.ts
@ -2,7 +2,7 @@ import * as plugins from '../../plugins.js';
 import { ForwardingHandler } from './base-handler.js';
 import type { IForwardConfig } from '../config/forwarding-types.js';
 import { ForwardingHandlerEvents } from '../config/forwarding-types.js';
-import { createSocketCleanupHandler, setupSocketHandlers, createSocketWithErrorHandler } from '../../core/utils/socket-utils.js';
+import { setupSocketHandlers, createSocketWithErrorHandler, setupBidirectionalForwarding } from '../../core/utils/socket-utils.js';

 /**
 * Handler for HTTPS termination with HTTPS backend
@ -96,17 +96,26 @@ export class HttpsTerminateToHttpsHandler extends ForwardingHandler {
    
    // Variable to track backend socket
    let backendSocket: plugins.tls.TLSSocket | null = null;
+    let isConnectedToBackend = false;
    
-    // Create cleanup handler for both sockets
-    const handleClose = createSocketCleanupHandler(tlsSocket, backendSocket, (reason) => {
-      this.emit(ForwardingHandlerEvents.DISCONNECTED, {
-        remoteAddress,
-        reason
-      });
-    });
+    // Set up initial error handling for TLS socket
+    const tlsCleanupHandler = (reason: string) => {
+      if (!isConnectedToBackend) {
+        // If backend not connected yet, just emit disconnected event
+        this.emit(ForwardingHandlerEvents.DISCONNECTED, {
+          remoteAddress,
+          reason
+        });
        
-    // Set up error handling with our cleanup utility
-    setupSocketHandlers(tlsSocket, handleClose, undefined, 'tls');
+        // Cleanup TLS socket if needed
+        if (!tlsSocket.destroyed) {
+          tlsSocket.destroy();
+        }
+      }
+      // If connected to backend, setupBidirectionalForwarding will handle cleanup
+    };
+    
+    setupSocketHandlers(tlsSocket, tlsCleanupHandler, undefined, 'tls');
    
    // Set timeout
    const timeout = this.getTimeout();
@ -117,7 +126,7 @@ export class HttpsTerminateToHttpsHandler extends ForwardingHandler {
        remoteAddress,
        error: 'TLS connection timeout'
      });
-      handleClose('timeout');
+      tlsCleanupHandler('timeout');
    });
    
    // Get the target from configuration
@ -131,44 +140,55 @@ export class HttpsTerminateToHttpsHandler extends ForwardingHandler {
        // In a real implementation, we would configure TLS options
        rejectUnauthorized: false // For testing only, never use in production
      }, () => {
+        isConnectedToBackend = true;
+        
        this.emit(ForwardingHandlerEvents.DATA_FORWARDED, {
          direction: 'outbound',
          target: `${target.host}:${target.port}`,
          tls: true
        });
        
-        // Set up bidirectional data flow
-        tlsSocket.pipe(backendSocket!);
-        backendSocket!.pipe(tlsSocket);
-      });
+        // Set up bidirectional forwarding with proper cleanup
+        setupBidirectionalForwarding(tlsSocket, backendSocket!, {
+          onCleanup: (reason) => {
+            this.emit(ForwardingHandlerEvents.DISCONNECTED, {
+              remoteAddress,
+              reason
+            });
+          },
+          enableHalfOpen: false // Close both when one closes
+        });
        
-      // Update the cleanup handler with the backend socket
-      const newHandleClose = createSocketCleanupHandler(tlsSocket, backendSocket, (reason) => {
-        this.emit(ForwardingHandlerEvents.DISCONNECTED, {
-          remoteAddress,
-          reason
+        // Set timeout for backend socket
+        backendSocket!.setTimeout(timeout);
+        
+        backendSocket!.on('timeout', () => {
+          this.emit(ForwardingHandlerEvents.ERROR, {
+            remoteAddress,
+            error: 'Backend connection timeout'
+          });
+          // Let setupBidirectionalForwarding handle the cleanup
        });
      });
      
-      // Set up handlers for backend socket
-      setupSocketHandlers(backendSocket, newHandleClose, undefined, 'backend');
-      
+      // Handle backend connection errors
      backendSocket.on('error', (error) => {
        this.emit(ForwardingHandlerEvents.ERROR, {
          remoteAddress,
          error: `Backend connection error: ${error.message}`
        });
-      });
        
-      // Set timeout for backend socket
-      backendSocket.setTimeout(timeout);
-      
-      backendSocket.on('timeout', () => {
-        this.emit(ForwardingHandlerEvents.ERROR, {
-          remoteAddress,
-          error: 'Backend connection timeout'
-        });
-        newHandleClose('backend_timeout');
+        if (!isConnectedToBackend) {
+          // Connection failed, clean up TLS socket
+          if (!tlsSocket.destroyed) {
+            tlsSocket.destroy();
+          }
+          this.emit(ForwardingHandlerEvents.DISCONNECTED, {
+            remoteAddress,
+            reason: `backend_connection_failed: ${error.message}`
+          });
+        }
+        // If connected, let setupBidirectionalForwarding handle cleanup
      });
    };