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.

Sign up for GitHub

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

Jump to bottom

Race condition in initializer #60

Closed
consp1racy opened this issue Apr 3, 2020 · 0 comments
Closed

Race condition in initializer #60

consp1racy opened this issue Apr 3, 2020 · 0 comments

Comments

@consp1racy
Copy link

SoLoader/java/com/facebook/soloader/SoLoader.java

Lines 201 to 203 in 6dce279

if (!NativeLoader.isInitialized()) {
NativeLoader.init(new NativeLoaderToSoLoaderDelegate());
}

If the initializer is called twice at the same time (e.g. SoLoader.init in Application.onCreate and Fresco.initialize on a worker thread) the app would crash with

java.lang.IllegalStateException: Cannot re-initialize NativeLoader.

Where Fresco checks NativeLoader.isInitialized and calls SoLoader.init:

https://github.com/facebook/fresco/blob/3c39beb47cb514d047eaaf9251badcf6f6db45ea/drawee-backends/drawee-pipeline/src/main/java/com/facebook/drawee/backends/pipeline/Fresco.java#L77-L102

https://github.com/facebook/fresco/blob/master/soloader/src/main/java/com/facebook/imagepipeline/nativecode/NativeCodeInitializer.java

Currently, there's nothing like NativeLoader.initIfNotAlready.
alsutton added a commit to alsutton/SoLoader that referenced this issue Jan 20, 2021
@alsutton
Add synchronized block to protect against race condition
f7e10d5 
Fixes: facebook#60
@alsutton alsutton mentioned this issue Jan 20, 2021
Closed
alsutton added a commit to alsutton/fresco that referenced this issue Jan 20, 2021
@alsutton
There is a period of time between checking if NativeLoader has been i…
7dcfe68 
…nitialized, and when we try to perform .init() which means a race condition can occur (as mentioned in facebook/SoLoader#60).

This change uses synchronization on the NativeLoader class, and a secondary check to verify that the initialization needs to take place just before we perform it, in order to reduce the window for a race condition to affect client code.

When combined with facebook/SoLoader#69 this will remove the race condition from the code because the initialisation inside SoLoader and inside Fresco will both be synchronized on the NativeLoad.class object.
@alsutton alsutton mentioned this issue Jan 20, 2021
Closed
4 tasks
facebook-github-bot pushed a commit to facebook/fresco that referenced this issue Jan 22, 2021
@alsutton @facebook-github-bot
Add synchronized to help prevent race condition with fresco client co…
00c035e 
…de (#2579)

Summary:
Thanks for submitting a PR! Please read these instructions carefully:

- [X] Explain the **motivation** for making this change.
- [X] Provide a **test plan** demonstrating that the code is solid.
- [X] Match the **code formatting** of the rest of the codebase.
- [X] Target the `master` branch

## Motivation (required)

There is a period of time between checking if NativeLoader has been initialized, and when we try to perform `.init()` which means a race condition can occur (as mentioned in facebook/SoLoader#60). This PR provides a mechanism for client code to avoid that race condition.

This change uses synchronization on the `NativeLoader` class object, and a secondary check to verify that the initialization needs to take place just before we perform it in order to reduce the window in which a race condition can happen.

When combined with facebook/SoLoader#69 this will remove the race condition from the code because the initialisation inside SoLoader and inside Fresco will both be synchronized on the `NativeLoader.class` object. Without that PR in SoLoader this change will still allow fresco clients to synchronize on `NativeLoader.class` in their own code around any areas which may race with this code in fresco.

## Test Plan (required)

This code doesn't alter the functionality of fresco, and so existing tests should provide verification that there is no operational impact.

## Next Steps

Sign the [CLA][2], if you haven't already.

Small pull requests are much easier to review and more likely to get merged. Make sure the PR does only one thing, otherwise please split it.

Make sure all **tests pass** on [Circle CI][4]. PRs that break tests are unlikely to be merged.

For more info, see the [Contributing guide][4].

[1]: https://medium.com/martinkonicek/what-is-a-test-plan-8bfc840ec171#.y9lcuqqi9
[2]: https://code.facebook.com/cla
[3]: http://circleci.com/gh/facebook/fresco
[4]: https://github.com/facebook/fresco/blob/master/CONTRIBUTING.md

Pull Request resolved: #2579

Reviewed By: oprisnik

Differential Revision: D25973346

Pulled By: wizh

fbshipit-source-id: 10239f7359e45ffc5deddec86d5798a838fc4bc5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add synchronized block to protect against race condition alsutton/SoLoader
1 participant
@consp1racy