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

Fix race-condition when loading local images #29595

Conversation

paddlefish
Copy link
Contributor

In local images, the callback is called inline, and so requestToken is always nil at the time the block is executed. In this case, the requestToken is irrelevant because the task is completed before it could be canceled. So initializing it with a do-nothing block satisfies the delegate.

It could also be a race condition even if the block is called on another thread, and so I also add @synchronized to protect against that race as well.

Fixes #29364 "Missing request token for request: <NSURLRequest: 0x60000253e5a0> { URL: file://"

Summary

Loading a local image will fail with an error "Missing request token for request:"

Changelog

[iOS] [Fixed] - Fix error thrown in RCTImageLoader.mm when accessing a file:// url

Test Plan

I have a react-native app which loads images from the camera roll using react-native-image-picker. When I added maxWidth: 480, maxHeight: 480, parameters this error started appearing. This code commit fixes that issue.

In local images, the callback is called inline, and so requestToken is always nil at the time the block is executed. In this case, the requestToken is irrelevant because the task is completed before it could be canceled. So initializing it with a do-nothing block satisfies the delegate.

It could also be a race condition even if the block is called on another thread, and so I also add @synchronized to protect against that race as well.

Fixes facebook#29364 "Missing request token for request: <NSURLRequest: 0x60000253e5a0> { URL: file://"
@facebook-github-bot
Copy link
Contributor

Hi @paddlefish!

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

@paddlefish
Copy link
Contributor Author

I agreed to the CLA

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 0416f77

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,840,116 0
android hermes armeabi-v7a 7,450,092 0
android hermes x86 8,298,434 0
android hermes x86_64 8,191,575 0
android jsc arm64-v8a 9,999,654 0
android jsc armeabi-v7a 9,601,761 0
android jsc x86 9,886,074 0
android jsc x86_64 10,465,403 0

Base commit: 0416f77

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 8, 2020
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

Copy link

@dclipca dclipca left a comment

Choose a reason for hiding this comment

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

Works perfectly!

@marsbergen
Copy link

Any plans when this PR will be part of the next patch release?

@danilobuerger
Copy link
Contributor

@shergin could you please review this? its impossible to upload local images anymore in the current rn version...

@barunprasad
Copy link

Any updates on this PR? Have been waiting for this to be merged for quite some time now.

@devkrkanhaiya
Copy link

devkrkanhaiya commented Sep 18, 2020

For IOS in
node_modules/react-native/Libraries/Image/RCTLocalAssetImageLoader.mm file

Replace Below

  • -(RCTImageLoaderCancellationBlock)loadImageForURL:(NSURL *)imageURL
    size:(CGSize)size
    scale:(CGFloat)scale
    resizeMode:(RCTResizeMode)resizeMode
    progressHandler:(RCTImageLoaderProgressBlock)progressHandler
    partialLoadHandler:(RCTImageLoaderPartialLoadBlock)partialLoadHandler
    completionHandler:(RCTImageLoaderCompletionBlock)completionHandler
    {
    UIImage *image = RCTImageFromLocalAssetURL(imageURL);
    if (image) {
    if (progressHandler) {
    progressHandler(1, 1);
    }
    completionHandler(nil, image);
    } else {
    NSString *message = [NSString stringWithFormat:@"Could not find image %@", imageURL];
    RCTLogWarn(@"%@", message);
    completionHandler(RCTErrorWithMessage(message), nil);
    }

return nil;
}

With

  • -(RCTImageLoaderCancellationBlock)loadImageForURL:(NSURL *)imageURL
    size:(CGSize)size
    scale:(CGFloat)scale
    resizeMode:(RCTResizeMode)resizeMode
    progressHandler:(RCTImageLoaderProgressBlock)progressHandler
    partialLoadHandler:(RCTImageLoaderPartialLoadBlock)partialLoadHandler
    completionHandler:(RCTImageLoaderCompletionBlock)completionHandler
    {
    __block auto cancelled = std::make_shared<std::atomic>(false);
    RCTExecuteOnMainQueue(^{
    if (cancelled->load()) {
    return;
    }

    UIImage *image = RCTImageFromLocalAssetURL(imageURL);
    if (image) {
    if (progressHandler) {
    progressHandler(1, 1);
    }
    completionHandler(nil, image);
    } else {
    NSString *message = [NSString stringWithFormat:@"Could not find image %@", imageURL];
    RCTLogWarn(@"%@", message);
    completionHandler(RCTErrorWithMessage(message), nil);
    }
    });

    return ^{
    cancelled->store(true);
    };
    }

This..

Like and Love , if it work 👍

@vincentjames501
Copy link

@shergin we hate to keep bothering you! This is affecting many folks (I see at least 5 different open tickets relating to this with comments from hundreds of folks). Seems like this is a simple fix/PR? Uploading images is completely broken on iOS via fetch and many applications depend on this functionality (we can't release our app until this one gets merged). Appreciate all your hard work!

@fairlycasual
Copy link

Any updates on timing for this being merged and released? We are midway through upgrading to 0.63.2 and this has become a blocker for us. We are not super comfortable with any of the work arounds presented, unless you can convince us of the stability for pegging package.json to use the nightly builds.

We appreciate all of your help and hard work on this, and many more issues!

@ThukuWakogi
Copy link

v0.63.3 is out and this hasn't been merged yet.

@honzajerabek
Copy link

honzajerabek commented Sep 30, 2020

@ThukuWakogi some ppl already reported that 0.63.3 fixes this issue. Haven’t tried it but there’s a chance 🤞

edit: it's fixed for me for uploading form-data image using axios

@AbleCable1
Copy link

Version 0.63.3 does not fix this issue, but this PR does. Would be good to get this merged soon. Currently we have to always manually modify the correct files in node_modules before building. It would be great to not have to do that every time

@fyujn
Copy link

fyujn commented Feb 10, 2021

Any updates on this?

Comment:
Upgraded to react-native version 0.63.4 works now in our app

@paddlefish
Copy link
Contributor Author

This should be closed. The issue was fixed another way

@paddlefish paddlefish closed this Oct 13, 2021
@paddlefish paddlefish deleted the arahn/image-loader-race-condition branch October 13, 2021 02:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing request token for request: <NSURLRequest: 0x60000253e5a0> { URL: file://