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

Remove BlockCacheFlush and improve InitCacheFlushTimeout mechanism #1642

Closed
mattjohnsonpint opened this issue May 5, 2022 · 2 comments · Fixed by #1644
Closed

Remove BlockCacheFlush and improve InitCacheFlushTimeout mechanism #1642

mattjohnsonpint opened this issue May 5, 2022 · 2 comments · Fixed by #1644
Assignees

Comments

@mattjohnsonpint
Copy link
Contributor

mattjohnsonpint commented May 5, 2022

The option InitCacheFlushTimeout controls how the BlockCacheFlush method works. Unfortunately, there are issues with this method, such as those raised in #1640. Mostly this is caused by sync-over-async antipattern.

I was going to rewrite most of it to be async, but after reviewing carefully, I'm fairly certain we don't need this feature at all. I'd like to remove it.

The only thing it does is pauses initialization on the calling thread for a certain amount of time while it tries to flush the cache in a background task. Either the cache is flushed within the timeout specified, or the timeout is reached and the flush keeps occurring in the background.

The reason I don't think we need it is that the CachingTransport already works in the background. As soon as it is created, it starts a background task and enters a loop, waiting for a signal that something is being sent. Then it processes items (same as flushing) before reseting the signal.

I propose we simply invert the loop. Always start with processing/flushing items, then wait for the signal that there's more items being sent. That will have the same effect as BlockFlushCache was doing, but without any need to worry about the InitCacheFlushTimeout and without any delay in completing initialization.

Associated PR forthcoming.

@bruno-garcia
Copy link
Member

bruno-garcia commented May 5, 2022

such as those raised in #1640. Mostly this is caused by sync-over-async antipattern.

Can we solve that by observing any exception thrown in the task?

The feature was built to make sure we can capture "Startup Crashes". Repro:

SentrySdk.Init(..);
throw null;

On mobile the SDK will be able to write to disk before closing (we block on the AppDomain UnhandledException integration) but it often can't flush the data over the network because the OS kills the app before it can do that.
So on restart, we can make sure we flush the cached file before we return from Init (synchronously, hence the sync over async).

More on: #1164

@mattjohnsonpint
Copy link
Contributor Author

Ok. I'll keep the behavior then, and the InitCacheFlushTimeout will stay. But I'll implement it differently, to avoid the issue raised and the async-over-sync stuff. The current implementation also has two tasks processing the cache simultaneously, one that's started by the worker in the caching transport, and the other that's started by BlockFlushCache. Best to only have one and just focus on the initialization timeout.

@mattjohnsonpint mattjohnsonpint changed the title Remove BlockCacheFlush and InitCacheFlushTimeout Remove BlockCacheFlush and improve InitCacheFlushTimeout mechanism May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants