Skip to content
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

fix: Re-work Sentry minidump loader #761

Merged
merged 12 commits into from
Oct 2, 2023

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented Sep 30, 2023

Closes #763

This PR re-writes the minidump loading code.

  • All the loading, validation, deleting and error catching logic is now all contained in createMinidumpLoader.
  • Removes possible race conditions from overlapping calls to lookup minidumps via our existing Mutex
  • Only reads minidumps from disk when they haven't been modified for 1 second
  • Skips sending minidumps if they're too small or don't start with the expected minidump header
  • Adds unit tests for the new logic

I had to extend the timings on many of the e2e tests because these changes add a delay in minidump sending while we wait to ensure the file is not being written.

@timfish timfish closed this Oct 1, 2023
@timfish timfish reopened this Oct 1, 2023
@timfish timfish force-pushed the fix/rework-minidump-loader branch 3 times, most recently from c427b78 to eb1f62b Compare October 1, 2023 10:53
@timfish timfish force-pushed the fix/rework-minidump-loader branch from eb1f62b to b38e05b Compare October 1, 2023 11:16
@timfish timfish force-pushed the fix/rework-minidump-loader branch from 7db467f to dfaf18c Compare October 1, 2023 23:14
@timfish timfish force-pushed the fix/rework-minidump-loader branch from dfaf18c to 329fabb Compare October 1, 2023 23:28
@timfish timfish marked this pull request as ready for review October 1, 2023 23:34
@timfish timfish force-pushed the fix/rework-minidump-loader branch from 329fabb to 380f2cc Compare October 1, 2023 23:49
@timfish timfish requested a review from AbhiPrasad October 2, 2023 00:13
@timfish timfish self-assigned this Oct 2, 2023
Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused at the extra session, but I think we should just ship since it doesn't break anything

@timfish
Copy link
Collaborator Author

timfish commented Oct 2, 2023

My best guess is that the extra session has always been there but we're only seeing it now after extending the app run time to cater for the sending delays added by this PR

@timfish timfish merged commit 7cf7b47 into getsentry:master Oct 2, 2023
41 checks passed
@timfish timfish deleted the fix/rework-minidump-loader branch October 2, 2023 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invalid Minidump
2 participants