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

Process isolation for cache directory #1095

Closed
wants to merge 5 commits into from
Closed

Conversation

Tyrrrz
Copy link
Contributor

@Tyrrrz Tyrrrz commented Jun 30, 2021

Closes #1067

Tyrrrz added 4 commits June 30, 2021 18:50
wip

Verified

This commit was signed with the committer’s verified signature.
sagikazarmark Márk Sági-Kazár
asd

Verified

This commit was signed with the committer’s verified signature.
sagikazarmark Márk Sági-Kazár
asd

Verified

This commit was signed with the committer’s verified signature.
sagikazarmark Márk Sági-Kazár
asd

Verified

This commit was signed with the committer’s verified signature.
sagikazarmark Márk Sági-Kazár
@codecov-commenter
Copy link

codecov-commenter commented Jun 30, 2021

Codecov Report

Merging #1095 (38d1856) into main (241ec58) will decrease coverage by 0.26%.
The diff coverage is 39.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1095      +/-   ##
==========================================
- Coverage   80.70%   80.43%   -0.27%     
==========================================
  Files         194      195       +1     
  Lines        6359     6405      +46     
  Branches     1411     1417       +6     
==========================================
+ Hits         5132     5152      +20     
- Misses        764      790      +26     
  Partials      463      463              
Impacted Files Coverage Δ
src/Sentry/Internal/Http/CachingTransport.cs 69.94% <33.33%> (-8.77%) ⬇️
src/Sentry/Internal/ProcessEx.cs 75.00% <75.00%> (ø)
src/Sentry/Internal/BackgroundWorker.cs 84.93% <0.00%> (+0.68%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 241ec58...38d1856. Read the comment docs.

@Tyrrrz
Copy link
Contributor Author

Tyrrrz commented Jun 30, 2021

How the fuck did this pass the tests

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

Just opened this and turns out I had unpublished reviews :)

rip alexey

options.CacheDirectoryPath,
"Sentry",
options.Dsn?.GetHashString() ?? "no-dsn"
)
: throw new InvalidOperationException("Cache directory is not set.");

_processingDirectoryPath = Path.Combine(_isolatedCacheDirectoryPath, "__processing");
_isolatedCacheDirectoryPath = Path.Combine(
Copy link
Member

Choose a reason for hiding this comment

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

Possibly an opt-out of this extra complexity given it isn't needed in mobile environments

{
public static int GetCurrentProcessId()
{
using var process = Process.GetCurrentProcess();
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't work on IL2CPP so we might need to swap out in that environment with some P/Invoke.

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.

Offline caching should support concurrent process instances
3 participants