ios: Use atomic renames instead of flock() in CrashReportDatabaseMac [crashpad/crashpad : main]
Justin Cohen (Gerrit)
Justin Cohen voted and added 1 comment![]( "Open in Gerrit")
Votes added by Justin Cohen
| Commit-Queue | +1 |
1 comment
OK -- this one dates back to a comment made by Ben Hamilton that has been living rent free in my head since 2022 (see https://chromium-review.git.corp.google.com/c/crashpad/crashpad/+/3868711/comments/529290ef_f69f163d)
What do you think of this change? Chrome (and all the other iOS crashpad clients) see quite a few watchdog kills for holding locks in shared containers.
Related details
- Mark Mentovai
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 2 comments![]( "Open in Gerrit")
OK -- this one dates back to a comment made by Ben Hamilton that has been living rent free in my head since 2022 (see https://chromium-review.git.corp.google.com/c/crashpad/crashpad/+/3868711/comments/529290ef_f69f163d)
What do you think of this change? Chrome (and all the other iOS crashpad clients) see quite a few watchdog kills for holding locks in shared containers.
OK -- this one dates back to a comment made by Ben Hamilton that has been living rent free in my head since 2022 (see https://chromium-review.git.corp.google.com/c/crashpad/crashpad/+/3868711/comments/529290ef_f69f163d)
Time to evict it?
constexpr char kProcessingDirectory[] = "processing";This introduces a brand new state, which would require a very careful review of several interfaces here, but I’m not going to do this because I think that this is actually infeasible.
How do things migrate out of the “processing” state if the uploader crashes (or the system loses power, etc.) while a report is in that state?
Related details
- Justin Cohen
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen added 1 comment![]( "Open in Gerrit")
constexpr char kProcessingDirectory[] = "processing";This introduces a brand new state, which would require a very careful review of several interfaces here, but I’m not going to do this because I think that this is actually infeasible.
How do things migrate out of the “processing” state if the uploader crashes (or the system loses power, etc.) while a report is in that state?
Thank you -- I appreciate that this would require a detailed review.
Reports migrate out of the processing state via `CleanDatabase(time_t lockfile_ttl)`
When `CleanDatabase()` runs, it scans the processing directory. Any report whose file modification time is older than `lockfile_ttl` is recovered by renaming it back to the pending directory. This mirrors the existing timeout-based recovery
mechanism used for expired lockfiles (client/crash_report_database_generic.cc lines 750–766) and stale new reports (client/crash_report_database_mac.mm lines 733–750.
In both `handler/prune_crash_reports_thread.cc` and `client/ios_handler/prune_intermediate_dumps_and_crash_reports_thread.cc` lockfile_ttl is set to 3 days.
Related details
- Mark Mentovai
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 1 comment![]( "Open in Gerrit")
constexpr char kProcessingDirectory[] = "processing";Justin CohenThis introduces a brand new state, which would require a very careful review of several interfaces here, but I’m not going to do this because I think that this is actually infeasible.
How do things migrate out of the “processing” state if the uploader crashes (or the system loses power, etc.) while a report is in that state?
Thank you -- I appreciate that this would require a detailed review.
Reports migrate out of the processing state via `CleanDatabase(time_t lockfile_ttl)`
When `CleanDatabase()` runs, it scans the processing directory. Any report whose file modification time is older than `lockfile_ttl` is recovered by renaming it back to the pending directory. This mirrors the existing timeout-based recovery
mechanism used for expired lockfiles (client/crash_report_database_generic.cc lines 750–766) and stale new reports (client/crash_report_database_mac.mm lines 733–750.In both `handler/prune_crash_reports_thread.cc` and `client/ios_handler/prune_intermediate_dumps_and_crash_reports_thread.cc` lockfile_ttl is set to 3 days.
Thank you -- I appreciate that this would require a detailed review.
Reports migrate out of the processing state via `CleanDatabase(time_t lockfile_ttl)`
When `CleanDatabase()` runs, it scans the processing directory. Any report whose file modification time is older than `lockfile_ttl` is recovered by renaming it back to the pending directory. This mirrors the existing timeout-based recovery
mechanism used for expired lockfiles (client/crash_report_database_generic.cc lines 750–766) and stale new reports (client/crash_report_database_mac.mm lines 733–750.In both `handler/prune_crash_reports_thread.cc` and `client/ios_handler/prune_intermediate_dumps_and_crash_reports_thread.cc` lockfile_ttl is set to 3 days.
- Justin Cohen
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen added 1 comment![]( "Open in Gerrit")
constexpr char kProcessingDirectory[] = "processing";Justin CohenThis introduces a brand new state, which would require a very careful review of several interfaces here, but I’m not going to do this because I think that this is actually infeasible.
How do things migrate out of the “processing” state if the uploader crashes (or the system loses power, etc.) while a report is in that state?
Mark MentovaiThank you -- I appreciate that this would require a detailed review.
Reports migrate out of the processing state via `CleanDatabase(time_t lockfile_ttl)`
When `CleanDatabase()` runs, it scans the processing directory. Any report whose file modification time is older than `lockfile_ttl` is recovered by renaming it back to the pending directory. This mirrors the existing timeout-based recovery
mechanism used for expired lockfiles (client/crash_report_database_generic.cc lines 750–766) and stale new reports (client/crash_report_database_mac.mm lines 733–750.In both `handler/prune_crash_reports_thread.cc` and `client/ios_handler/prune_intermediate_dumps_and_crash_reports_thread.cc` lockfile_ttl is set to 3 days.
Thank you -- I appreciate that this would require a detailed review.
Reports migrate out of the processing state via `CleanDatabase(time_t lockfile_ttl)`
When `CleanDatabase()` runs, it scans the processing directory. Any report whose file modification time is older than `lockfile_ttl` is recovered by renaming it back to the pending directory. This mirrors the existing timeout-based recovery
mechanism used for expired lockfiles (client/crash_report_database_generic.cc lines 750–766) and stale new reports (client/crash_report_database_mac.mm lines 733–750.In both `handler/prune_crash_reports_thread.cc` and `client/ios_handler/prune_intermediate_dumps_and_crash_reports_thread.cc` lockfile_ttl is set to 3 days.
I don’t love it. `flock` gives us immediate recovery and retry.
How about ten minutes? In client/settings.h we set the kUploadReportTimeoutSeconds to 20 seconds iOS and 60 seconds on macOS.
I can recover any "processing" reports older than 10 minutes back to pending.
Related details
- Mark Mentovai
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |