Avoid StrictMode warnings #4724
Conversation
…inishes Removed few useless IO calls
stefanosiano
requested review from
adinauer,
romtsn,
markushi and
lcian
as code owners
| directory.getAbsolutePath()); | ||
|
|
||
| for (File file : listFiles) { | ||
| for (File file : filteredListFiles) { |
There was a problem hiding this comment.
I found we were filtering the files in the directory, but then looped over all the files anyway
Was it by design or by accident?
And is there any risk in changing this?
There was a problem hiding this comment.
I'm pretty sure that was an oversight. IMHO the risk is low as all non-envelope files will fail to be parsed / sent anyway.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| ee747ae | 405.43 ms | 485.70 ms | 80.28 ms |
| ee747ae | 357.79 ms | 421.84 ms | 64.05 ms |
| 85d7417 | 347.21 ms | 394.35 ms | 47.15 ms |
| b750b96 | 408.98 ms | 480.32 ms | 71.34 ms |
| ee747ae | 358.21 ms | 389.41 ms | 31.20 ms |
| ee747ae | 415.92 ms | 470.15 ms | 54.23 ms |
| b750b96 | 421.25 ms | 444.09 ms | 22.84 ms |
| c8125f3 | 383.82 ms | 441.66 ms | 57.84 ms |
| ee747ae | 554.98 ms | 611.50 ms | 56.52 ms |
| 7314dbe | 437.83 ms | 505.64 ms | 67.81 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| 85d7417 | 1.58 MiB | 2.10 MiB | 533.44 KiB |
| b750b96 | 1.58 MiB | 2.10 MiB | 533.19 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| b750b96 | 1.58 MiB | 2.10 MiB | 533.20 KiB |
| c8125f3 | 1.58 MiB | 2.10 MiB | 532.32 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| 7314dbe | 1.58 MiB | 2.10 MiB | 533.45 KiB |
| directory.getAbsolutePath()); | ||
|
|
||
| for (File file : listFiles) { | ||
| for (File file : filteredListFiles) { |
There was a problem hiding this comment.
I'm pretty sure that was an oversight. IMHO the risk is low as all non-envelope files will fail to be parsed / sent anyway.
| @NotNull Sentry.OptionsConfiguration<SentryAndroidOptions> configuration) { | ||
| final @NotNull StrictMode.ThreadPolicy oldPolicy = StrictMode.getThreadPolicy(); | ||
| final @NotNull StrictMode.VmPolicy oldVmPolicy = StrictMode.getVmPolicy(); | ||
| StrictMode.setThreadPolicy(StrictMode.ThreadPolicy.LAX); |
There was a problem hiding this comment.
So I'm thinking this is too relaxing - e.g. the user init config (line 142) will also be under the LAX policy now, which is not what we want probably (e.g. if the user does I/O inside the init { } block this would not be flagged anymore). Can we be more specific and only wrap the actual offending calls into LAX?
There was a problem hiding this comment.
i was also thinking about resetting the policies before the config init (user callback) and then set it back to lax until the end of init()
Wdyt?
There was a problem hiding this comment.
that probably could do, but we're risking suppressing any new issues that might will have been introduced in the future outside the current ones. I'd really prefer to only target specific issues and set LAX as tight as possible, if it's not a big lift.
📜 Description
SentryAndroid.init now sets strictMode to Lax and reset it after it finishes
Removed few useless IO calls
💡 Motivation and Context
Closes #4261
💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps