When closing ZAP sometimes a system error shows up #1613
Conversation
- Believe that this was being caused by asynchronous reporting that we do for dirty flag and session notification counts on a periodic interval. When we close ZAP some of these reporting events are running queries on a closed database and thus throw a bunch of errors. The changes here stop the asynchronous reporting events from running when the database has been closed.
- JIRA: ZAPP-1607
There was a problem hiding this comment.
Summary of Changes
Hello @brdandu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request addresses a system error that occasionally occurs when ZAP is closing. The issue stemmed from asynchronous reporting tasks attempting to access the database after it had already been closed. My changes introduce a mechanism to explicitly mark the database as closed during shutdown and ensure that asynchronous reporting processes gracefully cease their operations, preventing these errors.
Highlights
- Database State Management: I've introduced a new
_closedproperty on the database object withindb-api.jsto explicitly mark when a database instance is being closed. This flag is set totruein both the asynchronouscloseDatabaseand synchronouscloseDatabaseSyncfunctions. - Graceful Asynchronous Reporting Shutdown: I've added calls to
asyncReporting.stopAsyncReporting()in bothcloseDatabaseandcloseDatabaseSyncfunctions. This ensures that any ongoing or scheduled asynchronous reporting tasks are notified to cease operations as the database is shutting down. - Preventing Database Access After Close: In
async-reporting.js, I've implemented checks for thedb._closedflag within thesetIntervalcallbacks for both session-based and non-session-based reports. If the database is marked as closed, these reporting functions will now immediately return, preventing attempts to query a closed database and thus avoiding errors.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request addresses an issue where asynchronous reporting tasks could query a closed database during shutdown, causing errors. The approach of adding a _closed flag to the database object and stopping the reporting tasks is sound.
However, the implementation has a couple of race conditions that could lead to unhandled promise rejections, potentially crashing the application. I've added two high-severity comments with suggestions to wrap the asynchronous logic inside setInterval callbacks with try...catch blocks to make the error handling more robust.
Once these suggestions are addressed, the change should effectively resolve the reported issue.
src-electron/util/async-reporting.js
Outdated
| report.id = setInterval(async () => { | ||
| // Check if the database is closed. Set in db-api#closeDatabase | ||
| if (db._closed) return | ||
| let sessions = await querySession.getAllSessions(db) | ||
| let allPromises = sessions.map((session) => report.fn(db, session)) | ||
| let allPromises = sessions.map((session) => { | ||
| if (db._closed) return | ||
| return report.fn(db, session) | ||
| }) | ||
| return Promise.all(allPromises) | ||
| }, report.intervalMs) |
There was a problem hiding this comment.
The async callback for setInterval can lead to an unhandled promise rejection if querySession.getAllSessions(db) fails (e.g., if the database is closed during the query). This can crash the application. It's crucial to wrap the asynchronous operations in a try...catch block to handle such race conditions gracefully.
report.id = setInterval(async () => {
try {
// Check if the database is closed. Set in db-api#closeDatabase
if (db._closed) return
let sessions = await querySession.getAllSessions(db)
let allPromises = sessions.map((session) => {
if (db._closed) return
return report.fn(db, session)
})
return Promise.all(allPromises)
} catch (err) {
// If the database was closed during an async operation, we can get an error.
// We can ignore it if the db is marked as closed.
if (db._closed) return
env.logWarning(`Error in session-based async reporting: ${err}`)
}
}, report.intervalMs)
src-electron/util/async-reporting.js
Outdated
| report.id = setInterval(() => { | ||
| if (db._closed) return | ||
| report.fn(db) | ||
| }, report.intervalMs) |
There was a problem hiding this comment.
The function report.fn is asynchronous and can reject, for example if the database is closed during a query. Since the returned promise is not handled, this will result in an unhandled promise rejection which can crash the application. The call should be awaited inside a try...catch block within an async callback to ensure stability.
report.id = setInterval(async () => {
if (db._closed) return
try {
await report.fn(db)
} catch (err) {
if (db._closed) return // Ignore errors if DB is closed.
env.logWarning(`Error in non-session-based async reporting: ${err}`)
}
}, report.intervalMs)426c6aa to
96acf69
Compare
There was a problem hiding this comment.
Not related to this PR, but * @param {*} intervalMs should be deleted in the function header comment.
- Believe that this was being caused by asynchronous reporting that we do for dirty flag and session notification counts on a periodic interval. When we close ZAP some of these reporting events are running queries on a closed database and thus throw a bunch of errors. The changes here stop the asynchronous reporting events from running when the database has been closed. - Taking AI's suggestion on the PR to put it in a try catch block - Adding error handling as suggested by the github AI assistant on the PR. The main reason db._closed is used is because when this chnage was not there and ZAP is closed/quit then a bunch of errors pop up. - JIRA: ZAPP-1607
aa0be77 to
825dfe5
Compare
Good catch |