From bd448b5108abe7812f4dd79b164b29dfe504990b Mon Sep 17 00:00:00 2001 From: fullex <0xfullex@gmail.com> Date: Sat, 13 Sep 2025 13:50:59 +0800 Subject: [PATCH] refactor: remove AbortSignal support from DataApiService and related components - Eliminated AbortSignal handling from ApiClient interface and DataApiService to streamline request processing. - Updated IpcAdapter to remove event sender logic for responses, aligning with the new direct IPC approach. - Adjusted tests to reflect the removal of cancellation capabilities, emphasizing that direct IPC requests cannot be cancelled. - Cleaned up related code and comments to enhance clarity and maintainability. --- packages/shared/data/api/apiSchemas.ts | 5 - src/main/data/api/core/adapters/IpcAdapter.ts | 8 +- src/preload/index.ts | 5 - src/renderer/src/data/DataApiService.ts | 274 ++++-------------- .../components/DataApiAdvancedTests.tsx | 60 ++-- .../components/DataApiStressTests.tsx | 48 ++- 6 files changed, 99 insertions(+), 301 deletions(-) diff --git a/packages/shared/data/api/apiSchemas.ts b/packages/shared/data/api/apiSchemas.ts index d333b1f4c7..e405af806e 100644 --- a/packages/shared/data/api/apiSchemas.ts +++ b/packages/shared/data/api/apiSchemas.ts @@ -395,7 +395,6 @@ export interface ApiClient { options?: { query?: QueryParamsForPath headers?: Record - signal?: AbortSignal } ): Promise> @@ -405,7 +404,6 @@ export interface ApiClient { body?: BodyForPath query?: Record headers?: Record - signal?: AbortSignal } ): Promise> @@ -415,7 +413,6 @@ export interface ApiClient { body: BodyForPath query?: Record headers?: Record - signal?: AbortSignal } ): Promise> @@ -424,7 +421,6 @@ export interface ApiClient { options?: { query?: Record headers?: Record - signal?: AbortSignal } ): Promise> @@ -434,7 +430,6 @@ export interface ApiClient { body?: BodyForPath query?: Record headers?: Record - signal?: AbortSignal } ): Promise> } diff --git a/src/main/data/api/core/adapters/IpcAdapter.ts b/src/main/data/api/core/adapters/IpcAdapter.ts index 4285d16d24..ee5e7ca345 100644 --- a/src/main/data/api/core/adapters/IpcAdapter.ts +++ b/src/main/data/api/core/adapters/IpcAdapter.ts @@ -29,7 +29,7 @@ export class IpcAdapter { logger.debug('Setting up IPC handlers...') // Main data request handler - ipcMain.handle(IpcChannel.DataApi_Request, async (event, request: DataRequest): Promise => { + ipcMain.handle(IpcChannel.DataApi_Request, async (_event, request: DataRequest): Promise => { try { logger.debug(`Handling data request: ${request.method} ${request.path}`, { id: request.id, @@ -38,9 +38,6 @@ export class IpcAdapter { const response = await this.apiServer.handleRequest(request) - // Send response back via Data_Response channel for async handling - event.sender.send(IpcChannel.DataApi_Response, response) - return response } catch (error) { logger.error(`Data request failed: ${request.method} ${request.path}`, error as Error) @@ -56,9 +53,6 @@ export class IpcAdapter { } } - // Send error response - event.sender.send(IpcChannel.DataApi_Response, errorResponse) - return errorResponse } }) diff --git a/src/preload/index.ts b/src/preload/index.ts index f2a40e9a2f..7c29d8579c 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -480,11 +480,6 @@ const api = { const listener = (_: any, data: any, event: string) => callback(data, event) ipcRenderer.on(channel, listener) return () => ipcRenderer.off(channel, listener) - }, - onResponse: (callback: (response: any) => void) => { - const listener = (_: any, response: any) => callback(response) - ipcRenderer.on(IpcChannel.DataApi_Response, listener) - return () => ipcRenderer.off(IpcChannel.DataApi_Response, listener) } } } diff --git a/src/renderer/src/data/DataApiService.ts b/src/renderer/src/data/DataApiService.ts index f044bf0fa9..5a49bc9bc9 100644 --- a/src/renderer/src/data/DataApiService.ts +++ b/src/renderer/src/data/DataApiService.ts @@ -33,15 +33,6 @@ interface RetryOptions { export class DataApiService implements ApiClient { private static instance: DataApiService private requestId = 0 - private pendingRequests = new Map< - string, - { - resolve: (value: DataResponse) => void - reject: (error: Error) => void - timeout?: NodeJS.Timeout - abortController?: AbortController - } - >() // Subscriptions private subscriptions = new Map< @@ -72,7 +63,7 @@ export class DataApiService implements ApiClient { } private constructor() { - this.setupResponseHandler() + // Initialization completed } /** @@ -85,30 +76,6 @@ export class DataApiService implements ApiClient { return DataApiService.instance } - /** - * Setup IPC response handler - */ - private setupResponseHandler() { - if (!window.api.dataApi?.onResponse) { - logger.error('Data API not available in preload context') - return - } - - window.api.dataApi.onResponse((response: DataResponse) => { - const pending = this.pendingRequests.get(response.id) - if (pending) { - clearTimeout(pending.timeout) - this.pendingRequests.delete(response.id) - - if (response.error) { - pending.reject(new Error(response.error.message)) - } else { - pending.resolve(response) - } - } - }) - } - /** * Generate unique request ID */ @@ -116,27 +83,6 @@ export class DataApiService implements ApiClient { return `req_${Date.now()}_${++this.requestId}` } - /** - * Cancel request by ID - */ - cancelRequest(requestId: string): void { - const pending = this.pendingRequests.get(requestId) - if (pending) { - pending.abortController?.abort() - clearTimeout(pending.timeout) - this.pendingRequests.delete(requestId) - pending.reject(new Error('Request cancelled')) - } - } - - /** - * Cancel all pending requests - */ - cancelAllRequests(): void { - const requestIds = Array.from(this.pendingRequests.keys()) - requestIds.forEach((id) => this.cancelRequest(id)) - } - /** * Configure retry options * @param options Partial retry options to override defaults @@ -158,106 +104,56 @@ export class DataApiService implements ApiClient { } /** - * Send request via IPC with AbortController support and retry logic + * Send request via IPC with direct return and retry logic */ private async sendRequest(request: DataRequest, retryCount = 0): Promise { - return new Promise((resolve, reject) => { - if (!window.api.dataApi.request) { - reject(new Error('Data API not available')) - return + if (!window.api.dataApi.request) { + throw new Error('Data API not available') + } + + try { + logger.debug(`Making ${request.method} request to ${request.path}`, { request }) + + // Direct IPC call with timeout + const response = await Promise.race([ + window.api.dataApi.request(request), + new Promise((_, reject) => setTimeout(() => reject(new Error(`Request timeout: ${request.path}`)), 3000)) + ]) + + if (response.error) { + throw new Error(response.error.message) } - // Create abort controller for cancellation - const abortController = new AbortController() - - // Set up timeout - const timeout = setTimeout(() => { - this.pendingRequests.delete(request.id) - const timeoutError = new Error(`Request timeout: ${request.path}`) - - // Check if should retry - if (retryCount < this.defaultRetryOptions.maxRetries && this.defaultRetryOptions.retryCondition(timeoutError)) { - logger.debug( - `Request timeout, retrying attempt ${retryCount + 1}/${this.defaultRetryOptions.maxRetries}: ${request.path}` - ) - - // Calculate delay with exponential backoff - const delay = - this.defaultRetryOptions.retryDelay * Math.pow(this.defaultRetryOptions.backoffMultiplier, retryCount) - - setTimeout(() => { - // Create new request with new ID for retry - const retryRequest = { ...request, id: this.generateRequestId() } - this.sendRequest(retryRequest, retryCount + 1) - .then(resolve) - .catch(reject) - }, delay) - } else { - reject(timeoutError) - } - }, 30000) // 30 second timeout - - // Store pending request with abort controller - this.pendingRequests.set(request.id, { - resolve: (response: DataResponse) => resolve(response.data), - reject: (error: Error) => { - // Check if should retry on error - if (retryCount < this.defaultRetryOptions.maxRetries && this.defaultRetryOptions.retryCondition(error)) { - logger.debug( - `Request failed, retrying attempt ${retryCount + 1}/${this.defaultRetryOptions.maxRetries}: ${request.path}`, - error - ) - - const delay = - this.defaultRetryOptions.retryDelay * Math.pow(this.defaultRetryOptions.backoffMultiplier, retryCount) - - setTimeout(() => { - const retryRequest = { ...request, id: this.generateRequestId() } - this.sendRequest(retryRequest, retryCount + 1) - .then(resolve) - .catch(reject) - }, delay) - } else { - reject(error) - } - }, - timeout, - abortController + logger.debug(`Request succeeded: ${request.method} ${request.path}`, { + status: response.status, + hasData: !!response.data }) - // Handle abort signal - abortController.signal.addEventListener('abort', () => { - clearTimeout(timeout) - this.pendingRequests.delete(request.id) - reject(new Error('Request cancelled')) - }) + return response.data as T + } catch (error) { + const errorMessage = error instanceof Error ? error.message : 'Unknown error' + logger.debug(`Request failed: ${request.method} ${request.path}`, error as Error) - // Send request - window.api.dataApi.request(request).catch((error) => { - clearTimeout(timeout) - this.pendingRequests.delete(request.id) + // Check if should retry + if (retryCount < this.defaultRetryOptions.maxRetries && this.defaultRetryOptions.retryCondition(error as Error)) { + logger.debug( + `Retrying request attempt ${retryCount + 1}/${this.defaultRetryOptions.maxRetries}: ${request.path}`, + { error: errorMessage } + ) - // Check if should retry - if (retryCount < this.defaultRetryOptions.maxRetries && this.defaultRetryOptions.retryCondition(error)) { - logger.debug( - `Request failed, retrying attempt ${retryCount + 1}/${this.defaultRetryOptions.maxRetries}: ${request.path}`, - error - ) + // Calculate delay with exponential backoff + const delay = + this.defaultRetryOptions.retryDelay * Math.pow(this.defaultRetryOptions.backoffMultiplier, retryCount) - const delay = - this.defaultRetryOptions.retryDelay * Math.pow(this.defaultRetryOptions.backoffMultiplier, retryCount) + await new Promise((resolve) => setTimeout(resolve, delay)) - setTimeout(() => { - const retryRequest = { ...request, id: this.generateRequestId() } - this.sendRequest(retryRequest, retryCount + 1) - .then(resolve) - .catch(reject) - }, delay) - } else { - reject(error) - } - }) - }) + // Create new request with new ID for retry + const retryRequest = { ...request, id: this.generateRequestId() } + return this.sendRequest(retryRequest, retryCount + 1) + } + + throw error + } } /** @@ -271,15 +167,9 @@ export class DataApiService implements ApiClient { body?: any headers?: Record metadata?: Record - signal?: AbortSignal } = {} ): Promise { - const { params, body, headers, metadata, signal } = options - - // Check if already aborted - if (signal?.aborted) { - throw new Error('Request cancelled') - } + const { params, body, headers, metadata } = options // Create request const request: DataRequest = { @@ -297,23 +187,7 @@ export class DataApiService implements ApiClient { logger.debug(`Making ${method} request to ${path}`, { request }) - // Set up external abort signal handling - const requestPromise = this.sendRequest(request) - - if (signal) { - // If external signal is aborted during request, cancel our internal request - const abortListener = () => { - this.cancelRequest(request.id) - } - signal.addEventListener('abort', abortListener) - - // Clean up listener when request completes - requestPromise.finally(() => { - signal.removeEventListener('abort', abortListener) - }) - } - - return requestPromise.catch((error) => { + return this.sendRequest(request).catch((error) => { logger.error(`Request failed: ${method} ${path}`, error) throw toDataApiError(error, `${method} ${path}`) }) @@ -327,13 +201,11 @@ export class DataApiService implements ApiClient { options?: { query?: any headers?: Record - signal?: AbortSignal } ): Promise { return this.makeRequest('GET', path as string, { params: options?.query, - headers: options?.headers, - signal: options?.signal + headers: options?.headers }) } @@ -346,14 +218,12 @@ export class DataApiService implements ApiClient { body?: any query?: Record headers?: Record - signal?: AbortSignal } ): Promise { return this.makeRequest('POST', path as string, { params: options.query, body: options.body, - headers: options.headers, - signal: options.signal + headers: options.headers }) } @@ -366,14 +236,12 @@ export class DataApiService implements ApiClient { body: any query?: Record headers?: Record - signal?: AbortSignal } ): Promise { return this.makeRequest('PUT', path as string, { params: options.query, body: options.body, - headers: options.headers, - signal: options.signal + headers: options.headers }) } @@ -385,13 +253,11 @@ export class DataApiService implements ApiClient { options?: { query?: Record headers?: Record - signal?: AbortSignal } ): Promise { return this.makeRequest('DELETE', path as string, { params: options?.query, - headers: options?.headers, - signal: options?.signal + headers: options?.headers }) } @@ -404,14 +270,12 @@ export class DataApiService implements ApiClient { body?: any query?: Record headers?: Record - signal?: AbortSignal } ): Promise { return this.makeRequest('PATCH', path as string, { params: options.query, body: options.body, - headers: options.headers, - signal: options.signal + headers: options.headers }) } @@ -471,28 +335,21 @@ export class DataApiService implements ApiClient { } /** - * Create an AbortController for request cancellation - * @returns Object with AbortController and convenience methods - * - * @example - * ```typescript - * const { signal, cancel } = requestService.createAbortController() - * - * // Use signal in requests - * const dataPromise = requestService.get('/topics', { signal }) - * - * // Cancel if needed - * setTimeout(() => cancel(), 5000) // Cancel after 5 seconds - * ``` + * Cancel request by ID + * Note: Direct IPC requests cannot be cancelled once sent + * @deprecated This method has no effect with direct IPC */ - createAbortController() { - const controller = new AbortController() + cancelRequest(requestId: string): void { + logger.warn('Request cancellation not supported with direct IPC', { requestId }) + } - return { - signal: controller.signal, - cancel: () => controller.abort(), - aborted: () => controller.signal.aborted - } + /** + * Cancel all pending requests + * Note: Direct IPC requests cannot be cancelled once sent + * @deprecated This method has no effect with direct IPC + */ + cancelAllRequests(): void { + logger.warn('Request cancellation not supported with direct IPC') } /** @@ -500,7 +357,7 @@ export class DataApiService implements ApiClient { */ getRequestStats() { return { - pendingRequests: this.pendingRequests.size, + pendingRequests: 0, // No longer tracked with direct IPC activeSubscriptions: this.subscriptions.size } } @@ -508,10 +365,3 @@ export class DataApiService implements ApiClient { // Export singleton instance export const dataApiService = DataApiService.getInstance() - -// Clean up on window unload -if (typeof window !== 'undefined') { - window.addEventListener('beforeunload', () => { - dataApiService.cancelAllRequests() - }) -} diff --git a/src/renderer/src/windows/dataRefactorTest/components/DataApiAdvancedTests.tsx b/src/renderer/src/windows/dataRefactorTest/components/DataApiAdvancedTests.tsx index 4805e32c05..2abe7d46c9 100644 --- a/src/renderer/src/windows/dataRefactorTest/components/DataApiAdvancedTests.tsx +++ b/src/renderer/src/windows/dataRefactorTest/components/DataApiAdvancedTests.tsx @@ -147,52 +147,28 @@ const DataApiAdvancedTests: React.FC = () => { setIsRunning(true) try { - // Test 1: Cancel a slow request - const slowTestPromise = runAdvancedTest('cancel-slow', 'Cancel Slow Request (5s)', 'cancellation', (signal) => - dataApiService.post('/test/slow', { body: { delay: 5000 }, signal }) - ) + // Note: Request cancellation is not supported with direct IPC + // These tests demonstrate that cancellation attempts have no effect - // Wait a bit then cancel - setTimeout(() => cancelTest('cancel-slow'), 2000) + // Test 1: Attempt to "cancel" a slow request (will complete normally) + await runAdvancedTest('cancel-slow', 'Slow Request (No Cancel Support)', 'cancellation', async () => { + // Note: This will complete normally since cancellation is not supported + return await dataApiService.post('/test/slow', { body: { delay: 1000 } }) + }) - try { - await slowTestPromise - } catch { - // Expected to be cancelled - } + // Test 2: Quick request (normal completion) + await runAdvancedTest('cancel-quick', 'Quick Request Test', 'cancellation', async () => { + return await dataApiService.get('/test/items') + }) - // Test 2: Cancel a request that should succeed - const quickTestPromise = runAdvancedTest('cancel-quick', 'Cancel Quick Request', 'cancellation', (signal) => - dataApiService.get('/test/items', { signal }) - ) + // Test 3: Demonstrate that cancel methods exist but have no effect + await runAdvancedTest('service-cancel', 'Cancel Method Test (No Effect)', 'cancellation', async () => { + // These methods exist but log warnings and have no effect + dataApiService.cancelRequest('dummy-id') + dataApiService.cancelAllRequests() - // Cancel immediately - setTimeout(() => cancelTest('cancel-quick'), 100) - - try { - await quickTestPromise - } catch { - // Expected to be cancelled - } - - // Test 3: Test using DataApiService's built-in cancellation - await runAdvancedTest('service-cancel', 'DataApiService Built-in Cancellation', 'cancellation', async () => { - const { signal, cancel } = dataApiService.createAbortController() - - // Start a slow request - const requestPromise = dataApiService.post('/test/slow', { body: { delay: 3000 }, signal }) - - // Cancel after 1 second - setTimeout(() => cancel(), 1000) - - try { - return await requestPromise - } catch (error) { - if (error instanceof Error && error.message.includes('cancelled')) { - return { cancelled: true, message: 'Successfully cancelled using DataApiService' } - } - throw error - } + // Return successful test data + return { cancelled: false, message: 'Cancel methods called but have no effect with direct IPC' } }) } finally { setIsRunning(false) diff --git a/src/renderer/src/windows/dataRefactorTest/components/DataApiStressTests.tsx b/src/renderer/src/windows/dataRefactorTest/components/DataApiStressTests.tsx index 3de8d28955..da82b79048 100644 --- a/src/renderer/src/windows/dataRefactorTest/components/DataApiStressTests.tsx +++ b/src/renderer/src/windows/dataRefactorTest/components/DataApiStressTests.tsx @@ -138,10 +138,7 @@ const DataApiStressTests: React.FC = () => { } }, [isRunning, updateRealtimeStats]) - const executeStressTest = async ( - testName: string, - testFn: (requestId: number, signal: AbortSignal) => Promise - ) => { + const executeStressTest = async (testName: string, testFn: (requestId: number) => Promise) => { setIsRunning(true) setCurrentTest(testName) setResults([]) @@ -188,7 +185,7 @@ const DataApiStressTests: React.FC = () => { setResults([...testResults]) try { - const response = await testFn(requestId, signal) + const response = await testFn(requestId) const endTime = Date.now() const duration = endTime - startTime const memoryAfter = getMemoryUsage() @@ -321,37 +318,30 @@ const DataApiStressTests: React.FC = () => { } const runConcurrentRequestsTest = async () => { - await executeStressTest('Concurrent Requests Test', async (requestId, signal) => { + await executeStressTest('Concurrent Requests Test', async (requestId) => { // Alternate between different endpoints to simulate real usage const endpoints = ['/test/items', '/test/stats'] as const const endpoint = endpoints[requestId % endpoints.length] - return await dataApiService.get(endpoint, { signal }) + return await dataApiService.get(endpoint) }) } const runMemoryLeakTest = async () => { - await executeStressTest('Memory Leak Test', async (requestId, signal) => { - // Create and immediately cancel some requests to test cleanup + await executeStressTest('Memory Leak Test', async (requestId) => { + // Test different types of requests to ensure no memory leaks if (requestId % 5 === 0) { - const { signal: cancelSignal, cancel } = dataApiService.createAbortController() - const requestPromise = dataApiService.post('/test/slow', { body: { delay: 2000 }, signal: cancelSignal }) - setTimeout(() => cancel(), 100) - - try { - return await requestPromise - } catch (error) { - return { cancelled: true, error: error instanceof Error ? error.message : 'Unknown error' } - } + // Test slower requests + return await dataApiService.post('/test/slow', { body: { delay: 500 } }) } else { // Normal request - return await dataApiService.get('/test/items', { signal }) + return await dataApiService.get('/test/items') } }) } const runErrorHandlingTest = async () => { - await executeStressTest('Error Handling Stress Test', async (requestId, signal) => { + await executeStressTest('Error Handling Stress Test', async (requestId) => { // Mix of successful and error requests const errorTypes = ['network', 'server', 'timeout', 'notfound', 'validation'] const shouldError = Math.random() * 100 < config.errorRate @@ -359,24 +349,24 @@ const DataApiStressTests: React.FC = () => { if (shouldError) { const errorType = errorTypes[requestId % errorTypes.length] try { - return await dataApiService.post('/test/error', { body: { errorType: errorType as any }, signal }) + return await dataApiService.post('/test/error', { body: { errorType: errorType as any } }) } catch (error) { return { expectedError: true, error: error instanceof Error ? error.message : 'Unknown error' } } } else { - return await dataApiService.get('/test/items', { signal }) + return await dataApiService.get('/test/items') } }) } const runMixedOperationsTest = async () => { - await executeStressTest('Mixed Operations Stress Test', async (requestId, signal) => { + await executeStressTest('Mixed Operations Stress Test', async (requestId) => { const operations = ['GET', 'POST', 'PUT', 'DELETE'] const operation = operations[requestId % operations.length] switch (operation) { case 'GET': - return await dataApiService.get('/test/items', { signal }) + return await dataApiService.get('/test/items') case 'POST': return await dataApiService.post('/test/items', { @@ -384,21 +374,19 @@ const DataApiStressTests: React.FC = () => { title: `Stress Test Topic ${requestId}`, description: `Created during stress test #${requestId}`, type: 'stress-test' - }, - signal + } }) case 'PUT': // First get an item, then update it try { - const items = await dataApiService.get('/test/items', { signal }) + const items = await dataApiService.get('/test/items') if ((items as any).items && (items as any).items.length > 0) { const itemId = (items as any).items[0].id return await dataApiService.put(`/test/items/${itemId}`, { body: { title: `Updated by stress test ${requestId}` - }, - signal + } }) } } catch (error) { @@ -407,7 +395,7 @@ const DataApiStressTests: React.FC = () => { return { updateFailed: true, message: 'No items found to update' } default: - return await dataApiService.get('/test/items', { signal }) + return await dataApiService.get('/test/items') } }) }