Skip to content

Commit

Permalink
Improve promise error handling (#196)
Browse files Browse the repository at this point in the history
* Add explicit catches and re-throw errors for every promise

This way the promise failures will hopefully become rejections of the
async functions they are in rather than "Uncaught (in promise)" errors
that bubble up to the top.

* Remove gitnews typedef file since we no longer use it

* Add more explicit catches

* Add try/catch to performFetch even though it should already be caught

* Add more logging to try to determine where the uncaught exception is
  • Loading branch information
sirbrillig authored Sep 13, 2024
1 parent 81f5099 commit 1c1d688
Show file tree
Hide file tree
Showing 4 changed files with 79 additions and 85 deletions.
2 changes: 1 addition & 1 deletion src/main/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ ipcMain.handle(
// data we actually want. See
// https://github.com/electron/electron/issues/24427
logMessage(
`Failure while fetching notifications for account ${account.name}(${account.serverUrl})`,
`Failure while fetching notifications for account ${account.name} (${account.serverUrl})`,
'error'
);
return {
Expand Down
36 changes: 23 additions & 13 deletions src/main/lib/github-interface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ async function getSubjectDataForNotification(
subjectHtmlUrl = subject.data.html_url;
} catch (error) {
logMessage(
`Failed to fetch comment for ${subjectPath} (${notification.subject.url})`,
`Failed to fetch subject for ${subjectPath} (${notification.subject.url})`,
'error'
);
}
Expand Down Expand Up @@ -252,21 +252,31 @@ export async function fetchNotificationsForAccount(
account,
notification
);
const promise = Promise.all([commentPromise, subjectPromise]);
const promise = Promise.all([commentPromise, subjectPromise]).catch(
(err) => {
throw err;
}
);
promises.push(promise);
promise.then(([commentData, subjectData]) => {
notes.push(
buildNoteFromData({
account,
notification,
subjectData,
commentData,
})
);
});
promise
.then(([commentData, subjectData]) => {
notes.push(
buildNoteFromData({
account,
notification,
subjectData,
commentData,
})
);
})
.catch((err) => {
throw err;
});
}

await Promise.all(promises);
await Promise.all(promises).catch((err) => {
throw err;
});

return notes;
}
54 changes: 0 additions & 54 deletions src/renderer/gitnews.d.ts

This file was deleted.

72 changes: 55 additions & 17 deletions src/renderer/lib/gitnews-fetcher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,21 +60,43 @@ export function createFetcher(): Middleware<{}, AppReduxState> {
'Accounts changed; fetching with updated accounts',
'info'
);
performFetch(
{
...store.getState(),
accounts: action.accounts,
},
next
);
try {
performFetch(
{
...store.getState(),
accounts: action.accounts,
},
next
);
} catch (err) {
window.electronApi.logMessage(
'Got an error fetching which somehow was not caught by the fetch handler',
'error'
);
console.error(
'Got an error fetching which somehow was not caught by the fetch handler',
err
);
}
return next(action);
}

// FIXME: why does this trigger so many times during a fetch?
if (action.type === 'GITNEWS_FETCH_NOTIFICATIONS') {
debug('Fetching accounts');
window.electronApi.logMessage('Fetching accounts', 'info');
performFetch(store.getState(), next);
try {
performFetch(store.getState(), next);
} catch (err) {
window.electronApi.logMessage(
'Got an error fetching which somehow was not caught by the fetch handler',
'error'
);
console.error(
'Got an error fetching which somehow was not caught by the fetch handler',
err
);
}
return;
}

Expand Down Expand Up @@ -114,8 +136,11 @@ export function createFetcher(): Middleware<{}, AppReduxState> {
// or the app will get stuck never updating again.
next(fetchBegin());

const getGithubNotifications = getFetcher(state.accounts, state.isDemoMode);
try {
const getGithubNotifications = getFetcher(
state.accounts,
state.isDemoMode
);
const notes = await getGithubNotifications();
debug('notifications retrieved', notes);
window.electronApi.logMessage(
Expand Down Expand Up @@ -153,17 +178,30 @@ export function createFetcher(): Middleware<{}, AppReduxState> {
`Fetching notifications for ${account.name} (${account.serverUrl})`,
'info'
);
const promise = fetchNotifications(account);
const promise = fetchNotifications(account)
.then((notes) => {
if ('error' in notes) {
throw notes.error;
}
allNotes = [...allNotes, ...notes];
})
.catch((err) => {
window.electronApi.logMessage(
`Fetching notifications FAILED for ${account.name} (${account.serverUrl})`,
'error'
);
throw err;
});
promises.push(promise);
promise.then((notes) => {
if ('error' in notes) {
throw notes.error;
}
allNotes = [...allNotes, ...notes];
});
}

await Promise.all(promises);
await Promise.all(promises).catch((err) => {
window.electronApi.logMessage(
`Waiting for fetched notifications FAILED`,
'error'
);
throw err;
});

allNotes.sort((a, b) => {
if (a.updatedAt < b.updatedAt) {
Expand Down

0 comments on commit 1c1d688

Please sign in to comment.