ref: retrying thread pool executor is a single thread executor instead of scheduled thread executor #422
Conversation
…d of scheduled thread executor
Codecov Report
@@ Coverage Diff @@
## master #422 +/- ##
============================================
- Coverage 60.19% 60.04% -0.15%
- Complexity 809 813 +4
============================================
Files 92 93 +1
Lines 3748 3742 -6
Branches 360 363 +3
============================================
- Hits 2256 2247 -9
- Misses 1338 1339 +1
- Partials 154 156 +2
Continue to review full report at Codecov.
|
sentry-android-core/src/main/java/io/sentry/android/core/DefaultAndroidEventProcessor.java
Outdated
Show resolved
Hide resolved
| @@ -96,6 +96,9 @@ public void store(final @NotNull SentryEnvelope envelope, final @Nullable Object | |||
| } | |||
|
|
|||
| if (hint instanceof SessionStart) { | |||
|
|
|||
| // TODO: should we move this to AppLifecycleIntegration? and do on SDK init? but it's too much | |||
There was a problem hiding this comment.
I'd rather not do it or we find a different way to improve things.
There was a problem hiding this comment.
fwiw on sentry-cocoa it's done on the thread calling init (expected to be the UI thread).
Agree it'd be best to avoid work on the UI thread but we need to make sure we don't race through the current session file which startSession would touch upon being called
There was a problem hiding this comment.
if I do that before even calling the Lifeycicle callback, no racing.
But sentry-android does more things, like the crash marker from sentry-native with a timestamp, so a hard crash can lead to a few IOs more on the main-thread and at this point, I'd say that we are doing quite a bit.
sentry-core/src/main/java/io/sentry/core/transport/QueuedThreadPoolExecutor.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
My points are mainly regarding other stuff that changed here but not really with the Single thread executor.
Also, it's worth noting now reading EnvelopeSender it does look like it needs refactoring.
Since we're unwrapping the envelope there and special casing things,maybe it's time to move that logic to the transport. In these Senders we just call captureEnvelope and in there we either call the envelope endpoint, or we unwrap them (code from current EnvelopeSender) to call the /store endpoint)
Main point to discuss is the Retryable hint bit. We need to be really conservative here because it can lead to different problems.
sentry-android-core/src/main/java/io/sentry/android/core/DefaultAndroidEventProcessor.java
Outdated
Show resolved
Hide resolved
| @@ -144,13 +146,18 @@ private void processEnvelope(final @NotNull SentryEnvelope envelope, final @Null | |||
| SentryLevel.WARNING, | |||
| "Timed out waiting for event submission: %s", | |||
| event.getEventId()); | |||
|
|
|||
| if (hint instanceof Retryable) { | |||
There was a problem hiding this comment.
H: Not sure about this. It could mean we're capturing events twice if we mark the event as retry=true and it actually finishes being sent. Why is this change introduced? We don't seem to have an issue missing events, right?
There was a problem hiding this comment.
it's a timed out and not a known exception that returns immediately which is 99.9% of the cases (no connection or something), so it really didn't have time enough due to different things, eg, long queue, very slow connection and low-end device, taking more time to serialize/deserialize, etc, I did reproduce this using emulator and very bad configurations purposely
There was a problem hiding this comment.
It times out waiting for the synchronization mechanism to be called as opposed to an HTTP request timeout. It's very possible that we'll give up waiting here before the HTTP request times out (unless we guarantee that the HTTP request times out before this waitFlush method here.
It's also an issue where have some bug in the SDK and now we're keeping things forever because this method is never called.
There was a problem hiding this comment.
I'm not talking about only http request timeout, it's altogether.
There was a problem hiding this comment.
I removed those Retryable checks from this PR and can come up with a new PR, merging this one without them
| @@ -96,6 +96,9 @@ public void store(final @NotNull SentryEnvelope envelope, final @Nullable Object | |||
| } | |||
|
|
|||
| if (hint instanceof SessionStart) { | |||
|
|
|||
| // TODO: should we move this to AppLifecycleIntegration? and do on SDK init? but it's too much | |||
There was a problem hiding this comment.
fwiw on sentry-cocoa it's done on the thread calling init (expected to be the UI thread).
Agree it'd be best to avoid work on the UI thread but we need to make sure we don't race through the current session file which startSession would touch upon being called
sentry-core/src/main/java/io/sentry/core/transport/AsyncConnection.java
Outdated
Show resolved
Hide resolved
yeah, I believe that when we get to the point of getting rid of /store, this refactoring will come naturally, what do you think?
replied on the comment. |
…dPoolExecutor.java
…randaneto/sentry-android into ref/retry-threadpool-executor
📢 Type of change
📜 Description
ref: retrying thread pool executor is a single thread pool executor instead of a scheduled thread pool executor.
💡 Motivation and Context
it executes tasks one after the other and we don't have scheduled tasks at all.
💚 How did you test it?
running it, tests passed.
📝 Checklist
🔮 Next steps