-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
fix(ui): Throw error if response is undefined in api client #94631
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
Conversation
Ignore the error if caught. I'm not sure how this happens but its probably some sort of browser extension or browser error. fetch should always return an object Response fixes JAVASCRIPT-31AQ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Error Handling Bypass Causes Memory Leak
The new if (response === undefined)
check throws an error that is then ignored by the catch
block. This bypasses the normal error handling flow, preventing the errorHandler
from being called (leading to silent failures for callers) and the completeHandler
from being called (causing a memory leak as requests are not removed from activeRequests
). Furthermore, the check does not account for response === null
, which would still cause a TypeError
during destructuring, resulting in inconsistent error handling.
static/app/api.tsx#L535-L659
Lines 535 to 659 in d1836a2
async response => { | |
if (response === undefined) { | |
// For some reason, response is undefined? Throw to the error path. | |
throw new Error('Response is undefined'); | |
} | |
// The Response's body can only be resolved/used at most once. | |
// So we clone the response so we can resolve the body content as text content. | |
// Response objects need to be cloned before its body can be used. | |
let responseJSON: any; | |
let responseText: any; | |
const {status, statusText} = response; | |
let {ok} = response; | |
let errorReason = 'Request not OK'; // the default error reason | |
let twoHundredErrorReason: string | undefined; | |
// Try to get text out of the response no matter the status | |
try { | |
responseText = await response.text(); | |
} catch (error) { | |
twoHundredErrorReason = 'Failed awaiting response.text()'; | |
ok = false; | |
if (error.name === 'AbortError') { | |
errorReason = 'Request was aborted'; | |
} else { | |
errorReason = error.toString(); | |
} | |
} | |
const responseContentType = response.headers.get('content-type'); | |
const isResponseJSON = responseContentType?.includes('json'); | |
const wasExpectingJson = | |
requestHeaders.get('Accept') === Client.JSON_HEADERS.Accept; | |
const isStatus3XX = status >= 300 && status < 400; | |
if (status !== 204 && !isStatus3XX) { | |
try { | |
responseJSON = JSON.parse(responseText); | |
} catch (error) { | |
twoHundredErrorReason = 'Failed trying to parse responseText'; | |
if (error.name === 'AbortError') { | |
ok = false; | |
errorReason = 'Request was aborted'; | |
} else if (isResponseJSON && error instanceof SyntaxError) { | |
// If the MIME type is `application/json` but decoding failed, | |
// this should be an error. | |
ok = false; | |
errorReason = 'JSON parse error'; | |
} else if ( | |
// Empty responses from POST 201 requests are valid | |
responseText?.length > 0 && | |
wasExpectingJson && | |
error instanceof SyntaxError | |
) { | |
// Was expecting json but was returned something else. Possibly HTML. | |
// Ideally this would not be a 200, but we should reject the promise | |
ok = false; | |
errorReason = 'JSON parse error. Possibly returned HTML'; | |
} | |
} | |
} | |
const responseMeta: ResponseMeta = { | |
status, | |
statusText, | |
responseJSON, | |
responseText, | |
getResponseHeader: (header: string) => response.headers.get(header), | |
}; | |
// Respect the response content-type header | |
const responseData = isResponseJSON ? responseJSON : responseText; | |
if (ok) { | |
successHandler(responseMeta, statusText, responseData); | |
} else { | |
// There's no reason we should be here with a 200 response, but we get | |
// tons of events from this codepath with a 200 status nonetheless. | |
// Until we know why, let's do what is essentially some very fancy print debugging. | |
if (status === 200 && responseText) { | |
const parameterizedPath = sanitizePath(path); | |
const message = '200 treated as error'; | |
Sentry.withScope(scope => { | |
scope.setTags({endpoint: `${method} ${parameterizedPath}`, errorReason}); | |
scope.setExtras({ | |
twoHundredErrorReason, | |
responseJSON, | |
responseText, | |
responseContentType, | |
errorReason, | |
}); | |
// Make sure all of these errors group, so we don't produce a bunch of noise | |
scope.setFingerprint([message]); | |
Sentry.captureException( | |
new Error(`${message}: ${method} ${parameterizedPath}`) | |
); | |
}); | |
} | |
const shouldSkipErrorHandler = globalErrorHandlers | |
.map(handler => handler(responseMeta, options)) | |
.some(Boolean); | |
if (!shouldSkipErrorHandler) { | |
errorHandler(responseMeta, statusText, errorReason); | |
} | |
} | |
completeHandler(responseMeta, statusText); | |
}, | |
() => { | |
// Ignore failed fetch calls or errors in the fetch request itself (e.g. cancelled requests) | |
// Not related to errors in responses | |
} | |
) | |
.catch(error => { | |
// eslint-disable-next-line no-console | |
console.error(error); | |
if (error?.name !== 'AbortError' && error?.message !== 'Response is undefined') { | |
Sentry.captureException(error); | |
} |
Was this report helpful? Give feedback by reacting with 👍 or 👎
Ignore the error if caught. I'm not sure how this happens but its probably some sort of browser extension or browser error. fetch should always return an object Response fixes JAVASCRIPT-31AQ - `Cannot destructure property 'status' of 'r' as it is undefined.`
Ignore the error if caught. I'm not sure how this happens but its probably some sort of browser extension or browser error. fetch should always return an object Response
fixes JAVASCRIPT-31AQ -
Cannot destructure property 'status' of 'r' as it is undefined.