Filestore thread safety fixes #295
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
moves code to delete files to single method within filestore
acquire a lock when writing/deleting files directly from the filestore
only deletes stored files if they are not already queued for a request
Pull Request Test Coverage Report for Build 1331
💛 - Coveralls |
1 similar comment
Pull Request Test Coverage Report for Build 1331
💛 - Coveralls |
adds tests for basic functionality of newly added methods
fractalwrench
changed the title
iterate through files and delete unqueued files if the max storage limit is reached
initialises a filestore directly in unit tests, rather than relying on the client. this should reduce side effects (e.g. error flushing on launch) which may have led to flakiness
provide a null check in the unlikely case the collection param is null
|
@bugsnag/platforms this is now ready for review. |
Cawllec
reviewed
Apr 26, 2018
| @@ -82,26 +100,63 @@ String write(@NonNull T streamable) { | |||
| filename), exception); | |||
| } finally { | |||
| IOUtils.closeQuietly(out); | |||
| lock.unlock(); | |||
There was a problem hiding this comment.
Is there a chance this can fail if IOUtils throws?
There was a problem hiding this comment.
NVM, I just looked up what IOUtils.closeQuietly does
Pezzah
reviewed
Apr 27, 2018
| } | ||
| return null; | ||
| } | ||
|
|
||
| @NonNull abstract String getFilename(T streamable); | ||
| @NonNull | ||
| abstract String getFilename(T streamable); | ||
|
|
||
| List<File> findStoredFiles() { |
There was a problem hiding this comment.
I think this can be called multiple times in the current design, which might cause bad things to happen. Potentially this should only return files that are not already in the queued files set?
There was a problem hiding this comment.
Good point. I've updated this method so that it only returns stored files which aren't in the queuedFiles collection.
finding stored files in filestore should now only return files which are not currently enqueued. this prevents bad things happening if a delivery client looks up files twice for example, and attempts to send duplicate reports.
Pezzah
approved these changes
Apr 27, 2018
lemnik
pushed a commit
that referenced
this pull request
Jun 2, 2021
Fix bugsnag.sharedObjectPaths not uploading SO files
rich-bugsnag
pushed a commit
that referenced
this pull request
Sep 3, 2021
…r-uplifts bugsnag-unity v5.0.0
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We sometimes receive invalid payloads with an empty/malformed array where an error/session should be - e.g.
[]or[,]. My hypothesis is that this occurs when the max limit of Error/Sessions are cached on disk. When the app is relaunched, ifwriteis called just before the payload is serialised, then this can lead to the oldest File being deleted, which means there is no data to serialise.This attempts to address this scenario by retaining a reference to any Files which are currently being flushed, and not deleting them.
A potential downside to this implementation is that it currently requires manually calling
cancelQueuedFilesanddeleteStoredFiles, which could be forgotten.