-
Notifications
You must be signed in to change notification settings - Fork 37
fix: harden desktop BLE — stale peripheral cache, state listener lifetime, acquire deadline #888
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
base: onekey
Are you sure you want to change the base?
Changes from 19 commits
0ccaa0d
006f903
d1fa81c
f6ceaf5
6f1d6ce
2544b0b
82a9616
0b81bf2
f15c0e2
881065c
4eff565
66fdd17
0631611
b1161c7
afed1c8
17d6012
8a6f829
ceb65f7
dc82ee3
d6be62d
3dd1b96
72e137f
dcbf3a7
aa45004
25e3854
64dcbd5
ce2d8e3
b2fa03f
25680af
ea906ff
b46341b
b2cb245
f177b02
12d79d1
be22a5d
294d4c3
081c18f
5df24cf
7148e31
dcdc438
bfb070f
37976e1
268ee94
cb79652
aaf7b7e
2e4b245
86d5a6b
106abb2
fd9b6e7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -415,6 +415,10 @@ const onCallDevice = async ( | |
| if (method.payload?.onlyConnectBleDevice) { | ||
|
originalix marked this conversation as resolved.
originalix marked this conversation as resolved.
originalix marked this conversation as resolved.
|
||
| preWarmCallbackTask?.resolve(); | ||
| Log.debug('Call API - only connect ble device: ', device?.mainId); | ||
| // This early return bypasses the normal-path releaseTask at the end of the | ||
| // call; without it the task leaks and haunts every later queue snapshot | ||
| // and cancel sweep (field log: a completed task lingered for 6 minutes). | ||
| requestQueue.releaseTask(method.responseID); | ||
|
originalix marked this conversation as resolved.
originalix marked this conversation as resolved.
originalix marked this conversation as resolved.
originalix marked this conversation as resolved.
|
||
| return createResponseMessage(method.responseID, true, null); | ||
| } | ||
|
|
||
|
|
@@ -954,7 +958,60 @@ export function isMissingDetectedProtocolV2Error(method: BaseMethod, error: unkn | |
| * If the Bluetooth connection times out, retry up to 6 times | ||
| * @param retryCount - Current retry count (default 0) | ||
| */ | ||
| async function connectDeviceForBle(method: BaseMethod, device: Device, retryCount = 0) { | ||
| // device.acquire awaits a transport reply with no deadline of its own; a | ||
| // transport that never settles (field case: Electron main lost an IPC reply, | ||
| // "reply was never sent" after 5 minutes) hangs the call forever and cancel() | ||
| // only takes effect at poll checkpoints. Race acquire against a deadline and | ||
| // the caller's abort signal so the hang is bounded and cancel is immediate. | ||
| const BLE_ACQUIRE_DEADLINE_MS = 60 * 1000; | ||
|
|
||
| function raceBleAcquire<T>(acquirePromise: Promise<T>, abortSignal?: AbortSignal): Promise<T> { | ||
| return new Promise<T>((resolve, reject) => { | ||
| let settled = false; | ||
| const settle = (fn: () => void) => { | ||
| if (settled) return; | ||
| settled = true; | ||
| clearTimeout(deadline); | ||
| abortSignal?.removeEventListener('abort', onAbort); | ||
| fn(); | ||
| }; | ||
| const onAbort = () => | ||
| settle(() => reject(ERRORS.TypedError(HardwareErrorCode.CallQueueActionCancelled))); | ||
| const deadline = setTimeout( | ||
| () => | ||
| settle(() => | ||
| reject( | ||
| ERRORS.TypedError( | ||
| HardwareErrorCode.BleTimeoutError, | ||
| `BLE acquire exceeded ${BLE_ACQUIRE_DEADLINE_MS}ms deadline` | ||
| ) | ||
| ) | ||
| ), | ||
| BLE_ACQUIRE_DEADLINE_MS | ||
| ); | ||
| // Attach before any early return so a late settlement of acquirePromise | ||
| // is always consumed — an abort or deadline must never leave the acquire | ||
| // rejection unhandled. | ||
| acquirePromise.then( | ||
| value => settle(() => resolve(value)), | ||
| error => settle(() => reject(error)) | ||
| ); | ||
| if (abortSignal) { | ||
| if (abortSignal.aborted) { | ||
| onAbort(); | ||
| return; | ||
| } | ||
| abortSignal.addEventListener('abort', onAbort); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| async function connectDeviceForBle( | ||
| method: BaseMethod, | ||
| device: Device, | ||
| abortSignal?: AbortSignal, | ||
| retryCount = 0 | ||
| ) { | ||
| try { | ||
| if (device.wasInterruptedByUser()) { | ||
| throw ERRORS.TypedError(HardwareErrorCode.DeviceInterruptedFromUser); | ||
|
|
@@ -968,9 +1025,43 @@ async function connectDeviceForBle(method: BaseMethod, device: Device, retryCoun | |
| !device.commands || | ||
| device.commands.disposed; | ||
| if (shouldAcquire) { | ||
| await device.acquire(method.payload.connectProtocol, { | ||
| forceProtocolDetection: method.payload.forceProtocolDetection, | ||
| }); | ||
| // The deadline/abort guards are scoped to the desktop electron | ||
| // transport: its IPC acquire is the only path with a proven | ||
| // never-settling failure mode, while react-native/lowlevel acquire may | ||
| // legitimately block on a user-driven system bonding prompt for longer | ||
| // than any sane deadline. Other envs keep the plain acquire unchanged. | ||
| const useAcquireGuards = DataManager.getSettings('env') === 'desktop-web-ble'; | ||
| // A cancel landing during the retry backoff must not start a new acquire. | ||
| if (useAcquireGuards && abortSignal?.aborted) { | ||
| throw ERRORS.TypedError(HardwareErrorCode.CallQueueActionCancelled); | ||
| } | ||
| if (!useAcquireGuards) { | ||
| await device.acquire(method.payload.connectProtocol, { | ||
| forceProtocolDetection: method.payload.forceProtocolDetection, | ||
| }); | ||
| } else { | ||
| try { | ||
| await raceBleAcquire( | ||
|
originalix marked this conversation as resolved.
originalix marked this conversation as resolved.
originalix marked this conversation as resolved.
originalix marked this conversation as resolved.
originalix marked this conversation as resolved.
originalix marked this conversation as resolved.
originalix marked this conversation as resolved.
originalix marked this conversation as resolved.
originalix marked this conversation as resolved.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: [Acquire deadline allows overlapping native acquires] When this deadline wins, only Please assign each acquire a cancellable generation, invalidate that generation on deadline, disconnect using the known BLE connect ID, and await teardown before retrying so a late settlement cannot update Device state.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: [Acquire deadline allows overlapping native acquires] When this deadline wins, only Please invalidate the Device connection-attempt generation, hard-disconnect |
||
| device.acquire(method.payload.connectProtocol, { | ||
| forceProtocolDetection: method.payload.forceProtocolDetection, | ||
| }), | ||
| abortSignal | ||
| ); | ||
| } catch (err) { | ||
| // A deadline hit means the transport is wedged mid-acquire; drop the | ||
| // link before the retry so it cold-connects instead of stacking a | ||
| // second connect onto the half-open one. | ||
| if ( | ||
|
originalix marked this conversation as resolved.
originalix marked this conversation as resolved.
originalix marked this conversation as resolved.
originalix marked this conversation as resolved.
|
||
| err.errorCode === HardwareErrorCode.BleTimeoutError && | ||
|
originalix marked this conversation as resolved.
originalix marked this conversation as resolved.
originalix marked this conversation as resolved.
|
||
| device.mainId && | ||
| device.deviceConnector | ||
| ) { | ||
| await device.deviceConnector.disconnect(device.mainId).catch(() => undefined); | ||
| device.markTransportDisconnected(); | ||
| } | ||
| throw err; | ||
| } | ||
| } | ||
| } | ||
| if (method.payload?.onlyConnectBleDevice) { | ||
| if (shouldAcquire) { | ||
|
|
@@ -1010,7 +1101,7 @@ async function connectDeviceForBle(method: BaseMethod, device: Device, retryCoun | |
| const nextRetry = retryCount + 1; | ||
| Log.debug(`Bluetooth connection will retry, retry count: ${nextRetry}`); | ||
| await wait(3000); | ||
| await connectDeviceForBle(method, device, nextRetry); | ||
| await connectDeviceForBle(method, device, abortSignal, nextRetry); | ||
| } else { | ||
| throw err; | ||
| } | ||
|
|
@@ -1120,7 +1211,7 @@ const ensureConnected = async ( | |
| if (tryCount === 1) { | ||
| device.beginConnectionAttempt(); | ||
| } | ||
| await connectDeviceForBle(method, device); | ||
| await connectDeviceForBle(method, device, abortSignal); | ||
| } | ||
| resolve(device); | ||
| return; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.