Skip to content
This repository was archived by the owner on Aug 30, 2023. It is now read-only.

feat: rotate cache folder #494

Merged
merged 5 commits into from
Jul 28, 2020
Merged

feat: rotate cache folder #494

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c677596
feat: rotate cache folder
marandaneto Jul 27, 2020
cfd4cb0
ref tests
marandaneto Jul 27, 2020
2807010
Update sentry-core/src/main/java/io/sentry/core/cache/CacheStrategy.java
marandaneto Jul 28, 2020
a2f1fac
code review
marandaneto Jul 28, 2020
696da07
make fields final
marandaneto Jul 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 94 additions & 0 deletions sentry-core/src/main/java/io/sentry/core/cache/CacheStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
package io.sentry.core.cache;

import static io.sentry.core.SentryLevel.ERROR;

import io.sentry.core.ISerializer;
import io.sentry.core.SentryLevel;
import io.sentry.core.SentryOptions;
import io.sentry.core.util.Objects;
import java.io.File;
import java.nio.charset.Charset;
import java.util.Arrays;
import org.jetbrains.annotations.NotNull;

abstract class CacheStrategy {
Copy link
Member

@philipphofmann philipphofmann Jul 27, 2020

Choose a reason for hiding this comment

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

m: I think we could split up this class into two classes. For me, we have two different responsibilities in here. We have some base functionality that both DiskCache and SessionCache need. And we have the sorting and deleting of the files. What about renaming CacheStrategy to something like SentryCache or BaseCache, although I'm not a fan of naming something base, ... and extracting sortFilesOldestToNewest and rotateCacheIfNeeded to a class called FileCacheStrategy or something similar. I'm a huge fan of composition over inheritance. So with the extracted class, we wouldn't have to create CustomCache in the CacheStrategyTest to be able to test the deleting and sorting of the files.

Copy link
Contributor Author

@marandaneto marandaneto Jul 27, 2020

Choose a reason for hiding this comment

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

not sure if it's worth creating another extra class to achieve this, actually only we wouldn't have to create CustomCache in the CacheStrategyTest is not enough argument as this is not added to the classpath, only on tests.

I still would need a class + an interface, ideally DiskCache and SessionCache will be merged very soon (when events become purely envelopes) and this class might lose its meaning, I mean, it'll be a single class anyway

opinions @bruno-garcia ?

Copy link
Member

@bruno-garcia bruno-garcia Jul 27, 2020

Choose a reason for hiding this comment

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

I'm also a huge fan of composition over inheritence but IMHO this is a good use of inheritance.

Also, this stuff is internal so no need to over engineer this and break it apart. If it grows in the upcoming changes, we can refactor then.

That said, IMHO LGTM

Copy link
Member

@philipphofmann philipphofmann Jul 28, 2020

Choose a reason for hiding this comment

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

I already stated my opinion and still with your arguments for keeping as it is I'm in favor of splitting it up, but I have no strong opinion on this as it's just a small class. I'm fine with keeping as it is. No need for further discussion IMO.


@SuppressWarnings("CharsetObjectCanBeUsed")
protected static final Charset UTF_8 = Charset.forName("UTF-8");

protected final @NotNull SentryOptions options;
protected final @NotNull ISerializer serializer;
protected final @NotNull File directory;
private final int maxSize;

CacheStrategy(
final @NotNull SentryOptions options,
final @NotNull String directoryPath,
final int maxSize) {
Objects.requireNonNull(directoryPath, "Directory is required.");
this.options = Objects.requireNonNull(options, "SentryOptions is required.");

this.serializer = options.getSerializer();
this.directory = new File(directoryPath);

this.maxSize = maxSize;
}

/**
* Check if a dir. is valid and have write and read permission
*
* @return true if valid and has permissions or false otherwise
*/
protected boolean isDirectoryValid() {
if (!directory.isDirectory() || !directory.canWrite() || !directory.canRead()) {
options
.getLogger()
.log(
ERROR,
"The directory for caching files is inaccessible.: %s",
directory.getAbsolutePath());
return false;
}
return true;
}

/**
* Sort files from oldest to the newest using the lastModified method
*
* @param files the Files
*/
private void sortFilesOldestToNewest(@NotNull File[] files) {
// just sort it if more than 1 file
if (files.length > 1) {
Arrays.sort(files, (f1, f2) -> Long.compare(f1.lastModified(), f2.lastModified()));
}
}

/**
* Rotates the caching folder if full, deleting the oldest files first
*
* @param files the Files
*/
protected void rotateCacheIfNeeded(final @NotNull File[] files) {
final int length = files.length;
if (length >= maxSize) {
options
.getLogger()
.log(SentryLevel.WARNING, "Cache folder if full (respecting maxSize). Rotating files");
final int totalToBeDeleted = (length - maxSize) + 1;

sortFilesOldestToNewest(files);

// delete files from the top of the Array as its sorted by the oldest to the newest
for (int i = 0; i < totalToBeDeleted; i++) {
final File file = files[i];
// sanity check if the file actually exists.
if (!file.delete()) {
options
.getLogger()
.log(SentryLevel.WARNING, "File can't be deleted: %s", file.getAbsolutePath());
}
}
}
}
}
73 changes: 17 additions & 56 deletions sentry-core/src/main/java/io/sentry/core/cache/DiskCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,8 @@
import static io.sentry.core.SentryLevel.WARNING;
import static java.lang.String.format;

import io.sentry.core.ISerializer;
import io.sentry.core.SentryEvent;
import io.sentry.core.SentryLevel;
import io.sentry.core.SentryOptions;
import io.sentry.core.util.Objects;
import java.io.BufferedReader;
import java.io.BufferedWriter;
import java.io.File;
Expand All @@ -22,7 +19,6 @@
import java.io.OutputStreamWriter;
import java.io.Reader;
import java.io.Writer;
import java.nio.charset.Charset;
import java.util.ArrayList;
import java.util.Iterator;
import java.util.List;
Expand All @@ -34,39 +30,19 @@
* configured directory.
*/
@ApiStatus.Internal
public final class DiskCache implements IEventCache {
public final class DiskCache extends CacheStrategy implements IEventCache {
/** File suffix added to all serialized event files. */
public static final String FILE_SUFFIX = ".sentry-event";

@SuppressWarnings("CharsetObjectCanBeUsed")
private static final Charset UTF_8 = Charset.forName("UTF-8");

private final File directory;
private final int maxSize;
private final ISerializer serializer;
private final SentryOptions options;

public DiskCache(SentryOptions options) {
Objects.requireNonNull(options.getCacheDirPath(), "Cache dir. path is required.");
this.directory = new File(options.getCacheDirPath());
this.maxSize = options.getCacheDirSize();
this.serializer = options.getSerializer();
this.options = options;
public DiskCache(final @NotNull SentryOptions options) {
super(options, options.getCacheDirPath(), options.getCacheDirSize());
}

@Override
public void store(SentryEvent event) {
if (getNumberOfStoredEvents() >= maxSize) {
options
.getLogger()
.log(
SentryLevel.WARNING,
"Disk cache full (respecting maxSize). Not storing event {}",
event);
return;
}
public void store(final @NotNull SentryEvent event) {
rotateCacheIfNeeded(allEventFiles());

File eventFile = getEventFile(event);
final File eventFile = getEventFile(event);
if (eventFile.exists()) {
options
.getLogger()
Expand All @@ -92,8 +68,8 @@ public void store(SentryEvent event) {
}

@Override
public void discard(SentryEvent event) {
File eventFile = getEventFile(event);
public void discard(final @NotNull SentryEvent event) {
final File eventFile = getEventFile(event);
if (eventFile.exists()) {
options
.getLogger()
Expand All @@ -107,34 +83,17 @@ public void discard(SentryEvent event) {
}
}

private int getNumberOfStoredEvents() {
return allEventFiles().length;
}

private boolean isDirectoryValid() {
if (!directory.isDirectory() || !directory.canWrite() || !directory.canRead()) {
options
.getLogger()
.log(
ERROR,
"The directory for caching Sentry events is inaccessible.: %s",
directory.getAbsolutePath());
return false;
}
return true;
}

private File getEventFile(SentryEvent event) {
private @NotNull File getEventFile(final @NotNull SentryEvent event) {
return new File(directory.getAbsolutePath(), event.getEventId().toString() + FILE_SUFFIX);
}

@Override
public @NotNull Iterator<SentryEvent> iterator() {
File[] allCachedEvents = allEventFiles();
final File[] allCachedEvents = allEventFiles();

List<SentryEvent> ret = new ArrayList<>(allCachedEvents.length);
final List<SentryEvent> ret = new ArrayList<>(allCachedEvents.length);

for (File f : allCachedEvents) {
for (final File f : allCachedEvents) {
try (final Reader reader =
new BufferedReader(new InputStreamReader(new FileInputStream(f), UTF_8))) {

Expand All @@ -159,10 +118,12 @@ private File getEventFile(SentryEvent event) {
return ret.iterator();
}

private File[] allEventFiles() {
private @NotNull File[] allEventFiles() {
if (isDirectoryValid()) {
// TODO: we need to order by oldest to the newest here
return directory.listFiles((__, fileName) -> fileName.endsWith(FILE_SUFFIX));
final File[] files = directory.listFiles((__, fileName) -> fileName.endsWith(FILE_SUFFIX));
if (files != null) {
return files;
}
}
return new File[] {};
}
Expand Down
53 changes: 9 additions & 44 deletions sentry-core/src/main/java/io/sentry/core/cache/SessionCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
import static java.lang.String.format;

import io.sentry.core.DateUtils;
import io.sentry.core.ISerializer;
import io.sentry.core.SentryEnvelope;
import io.sentry.core.SentryEnvelopeItem;
import io.sentry.core.SentryItemType;
Expand All @@ -33,7 +32,6 @@
import java.io.OutputStreamWriter;
import java.io.Reader;
import java.io.Writer;
import java.nio.charset.Charset;
import java.util.ArrayList;
import java.util.Date;
import java.util.Iterator;
Expand All @@ -46,7 +44,7 @@
import org.jetbrains.annotations.Nullable;

@ApiStatus.Internal
public final class SessionCache implements IEnvelopeCache {
public final class SessionCache extends CacheStrategy implements IEnvelopeCache {

/** File suffix added to all serialized envelopes files. */
static final String SUFFIX_ENVELOPE_FILE = ".envelope";
Expand All @@ -55,37 +53,17 @@ public final class SessionCache implements IEnvelopeCache {
static final String SUFFIX_CURRENT_SESSION_FILE = ".json";
static final String CRASH_MARKER_FILE = ".sentry-native/last_crash";

@SuppressWarnings("CharsetObjectCanBeUsed")
private static final Charset UTF_8 = Charset.forName("UTF-8");

private final @NotNull File directory;
private final int maxSize;
private final @NotNull ISerializer serializer;
private final @NotNull SentryOptions options;

private final @NotNull Map<SentryEnvelope, String> fileNameMap = new WeakHashMap<>();

public SessionCache(final @NotNull SentryOptions options) {
Objects.requireNonNull(options.getSessionsPath(), "sessions dir. path is required.");
this.directory = new File(options.getSessionsPath());
this.maxSize = options.getSessionsDirSize();
this.serializer = options.getSerializer();
this.options = options;
super(options, options.getSessionsPath(), options.getSessionsDirSize());
}

@Override
public void store(final @NotNull SentryEnvelope envelope, final @Nullable Object hint) {
Objects.requireNonNull(envelope, "Envelope is required.");

if (getNumberOfStoredEnvelopes() >= maxSize) {
options
.getLogger()
.log(
SentryLevel.WARNING,
"Disk cache full (respecting maxSize). Not storing envelope {}",
envelope);
return;
}
rotateCacheIfNeeded(allEnvelopeFiles());

final File currentSessionFile = getCurrentSessionFile();

Expand Down Expand Up @@ -185,7 +163,7 @@ public void store(final @NotNull SentryEnvelope envelope, final @Nullable Object
* @param markerFile the marker file
* @return the timestamp as Date
*/
private Date getTimestampFromCrashMarkerFile(final @NotNull File markerFile) {
private @Nullable Date getTimestampFromCrashMarkerFile(final @NotNull File markerFile) {
try (final BufferedReader reader =
new BufferedReader(new InputStreamReader(new FileInputStream(markerFile), UTF_8))) {
final String timestamp = reader.readLine();
Expand Down Expand Up @@ -299,23 +277,6 @@ public void discard(final @NotNull SentryEnvelope envelope) {
}
}

private int getNumberOfStoredEnvelopes() {
return allEnvelopeFiles().length;
}

private boolean isDirectoryValid() {
if (!directory.isDirectory() || !directory.canWrite() || !directory.canRead()) {
options
.getLogger()
.log(
ERROR,
"The directory for caching Sentry envelopes is inaccessible.: %s",
directory.getAbsolutePath());
return false;
}
return true;
}

/**
* Returns the envelope's file path. If the envelope has no eventId header, it generates a random
* file name to it.
Expand Down Expand Up @@ -378,7 +339,11 @@ private boolean isDirectoryValid() {
private @NotNull File[] allEnvelopeFiles() {
if (isDirectoryValid()) {
// lets filter the session.json here
return directory.listFiles((__, fileName) -> fileName.endsWith(SUFFIX_ENVELOPE_FILE));
final File[] files =
directory.listFiles((__, fileName) -> fileName.endsWith(SUFFIX_ENVELOPE_FILE));
if (files != null) {
return files;
}
}
return new File[] {};
}
Expand Down
Loading