-
-
Notifications
You must be signed in to change notification settings - Fork 476
perf(core): Create outbox and cache dirs lazily instead of during init (JAVA-613) #5792
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
base: main
Are you sure you want to change the base?
Changes from all commits
429252a
0ba52d9
435f985
648d7f2
6b422c3
2dfe4e1
9e04ed7
86670d6
bb93d87
1c44d83
eda3158
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -93,7 +93,7 @@ private boolean storeInternalAndroid(@NotNull SentryEnvelope envelope, @NotNull | |
|
|
||
| @TestOnly | ||
| public @NotNull File getDirectory() { | ||
| return directory; | ||
| return directory.getFile(); | ||
| } | ||
|
|
||
| private void writeStartupCrashMarkerFile() { | ||
|
|
@@ -106,7 +106,14 @@ private void writeStartupCrashMarkerFile() { | |
| .log(DEBUG, "Outbox path is null, the startup crash marker file will not be written"); | ||
| return; | ||
| } | ||
| final File crashMarkerFile = new File(outboxPath, STARTUP_CRASH_MARKER_FILE); | ||
| // The outbox dir is no longer created during Sentry.init, so create it here in case the native | ||
| // SDK (which normally creates it) is disabled. | ||
| final File outboxDir = new File(outboxPath); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Longer-term side note: Seems like That'd^^ let us centralize our policies + avoid having to check for existence (etc.) in multiple places. (Obviously way out of scope here, but something for us to think about.)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sounds interesting! probably related to this two phase init idea: #5704 |
||
| if (!FileUtils.createDirectory(outboxDir)) { | ||
| options.getLogger().log(ERROR, "Failed to create outbox dir %s", outboxPath); | ||
| return; | ||
| } | ||
| final File crashMarkerFile = new File(outboxDir, STARTUP_CRASH_MARKER_FILE); | ||
| try { | ||
| crashMarkerFile.createNewFile(); | ||
| } catch (Throwable e) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ | |
| import java.io.FileReader; | ||
| import java.io.IOException; | ||
| import org.jetbrains.annotations.ApiStatus; | ||
| import org.jetbrains.annotations.NotNull; | ||
| import org.jetbrains.annotations.Nullable; | ||
|
|
||
| @ApiStatus.Internal | ||
|
|
@@ -36,6 +37,21 @@ public static boolean deleteRecursively(@Nullable File file) { | |
| return file.delete(); | ||
| } | ||
|
|
||
| /** | ||
| * Creates the directory and any missing parents, if it does not exist yet. | ||
| * | ||
| * <p>Callers are expected to log a failure: a missing directory otherwise surfaces later as an | ||
| * unrelated-looking write error. | ||
| * | ||
| * @param directory the directory to create | ||
| * @return true if the directory exists once this returns, false if it could not be created | ||
| */ | ||
| public static boolean createDirectory(final @NotNull File directory) { | ||
| // mkdirs() also returns false when another thread created the directory first, so re-check | ||
| // instead of reporting a failure the caller would act on by skipping its write. | ||
| return directory.isDirectory() || directory.mkdirs() || directory.isDirectory(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not gonna lie this reads super confusing ๐ |
||
| } | ||
|
cursor[bot] marked this conversation as resolved.
|
||
|
|
||
| /** | ||
| * Reads the content of a File into a String. If the file does not exist or is not a file, null is | ||
| * returned. Do not use with large files, as the String is kept in memory! | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| package io.sentry.util; | ||
|
|
||
| import java.io.File; | ||
| import org.jetbrains.annotations.ApiStatus; | ||
| import org.jetbrains.annotations.NotNull; | ||
|
|
||
| /** | ||
| * A filesystem directory that is created on demand rather than up front, so the (potentially | ||
| * blocking) {@code mkdirs()} runs on the thread that first writes into it instead of on the SDK | ||
| * init thread. | ||
| * | ||
| * <p>Read paths should use {@link #getFile()}, which never touches the filesystem. Write paths | ||
| * should call {@link #getOrCreate()} once before writing. Creation is not cached: on Android the | ||
| * cache dir lives under {@code Context.getCacheDir()}, which the system may wipe at any time, so | ||
| * each write re-checks. | ||
| */ | ||
| @ApiStatus.Internal | ||
| public final class LazyDirectory { | ||
|
|
||
| private final @NotNull File file; | ||
|
|
||
| public LazyDirectory(final @NotNull String path) { | ||
| this.file = new File(path); | ||
| } | ||
|
|
||
| /** Returns the directory without touching the filesystem. */ | ||
| public @NotNull File getFile() { | ||
| return file; | ||
| } | ||
|
|
||
| /** Returns the directory, creating it and any missing parents if it does not exist yet. */ | ||
| public @NotNull File getOrCreate() { | ||
| // A failed mkdirs is not reported here: callers are write paths, so the failure surfaces as the | ||
| // write error they already log and report. | ||
| FileUtils.createDirectory(file); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. m: We're ignoring a possible
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a very good catch. To be fair, the previous code would also ignore the return value of |
||
| return file; | ||
| } | ||
| } | ||
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.
m: We should also mention that direct outbox writers now need to create the outbox directory before writing envelopes.
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.
I feel like this might confuse consumers of the SDK but I'll do this. I think it makes sense to put in the release notes.