-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
Clear _handlers on RCTNetworking invalidation #19169
Clear _handlers on RCTNetworking invalidation #19169
Conversation
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 up 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 the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
Generated by 🚫 dangerJS |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
Thanks for the PR! Code looks good. Could you just add a Test Plan and Release Notes section to your PR description, see the PR template for more details about what it should look like. For a PR like this you can simply explain quickly how you tested the change and can mark it as Internal for the release notes. |
@janicduplessis done, also added repro steps for you guys to confirm. Thanks |
Thanks! @facebook-github-bot shipit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@janicduplessis is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@oNaiPs, thanks for the PR! Was wondering if you've seen this issue I'm running into before and if you think it'd be related to the problem you've tried fixing? Basically, I'm seeing a lot of memory getting allocated for the |
@yinghang there could be a lot of reasons for that to happen. Ping me if you can get a way of reproducing it with a minimal sample code. |
@oNaiPs Not sure if you got a notification saying that I tagged you in a new issue, but I've created a new issue regarding this, as well as the accompanying minimal sample code. Thanks for your help in advance! 😄 |
Thanks @yinghang I did see it :) did not have the time to look into the details though, but realized this week that this exception I fixed came back again. I am pretty busy these days but will try to look into it when possible (if the FB guys won't) |
Summary: This PR fixes a bug where in `RCTNetworking` not all tasks/handlers were not being cleared when invalidating the class. I came across this issue when writing some unit tests for my native plugins, sometimes a test would finish running (and the bridge invalidated), and only afterwards a callback from RCTNetworking would come, resulting in this exception: ``` 2018-05-07 15:23:34.264494-0700 Guardian[73794:10710945] *** Assertion failure in -[RCTEventEmitter sendEventWithName:body:](), /Users/.../app/node_modules/react-native/React/Modules/RCTEventEmitter.m:41 2018-05-07 15:23:34.276505-0700 Guardian[73794:10710945] *** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'Error when sending event: didCompleteNetworkResponse with body: ( 2, cancelled, 0 ). Bridge is not set. This is probably because you've explicitly synthesized the bridge in RCTNetworking, even though it's inherited from RCTEventEmitter.' *** First throw call stack: ( 0 CoreFoundation 0x000000010d5b21e6 __exceptionPreprocess + 294 1 libobjc.A.dylib 0x000000010be6f031 objc_exception_throw + 48 2 CoreFoundation 0x000000010d5b7472 +[NSException raise:format:arguments:] + 98 3 Foundation 0x000000010b94864f -[NSAssertionHandler handleFailureInFunction:file:lineNumber:description:] + 165 4 Guardian 0x0000000106ff5227 -[RCTEventEmitter sendEventWithName:body:] + 567 5 Guardian 0x0000000106e9ebab __76-[RCTNetworking sendRequest:responseType:incrementalUpdates:responseSender:]_block_invoke.423 + 1115 6 Guardian 0x0000000106e8f48c __50-[RCTNetworkTask URLRequest:didCompleteWithError:]_block_invoke + 92 7 Guardian 0x0000000106e8ded1 -[RCTNetworkTask dispatchCallback:] + 113 8 Guardian 0x0000000106e8f37a -[RCTNetworkTask URLRequest:didCompleteWithError:] + 410 9 Guardian 0x0000000106ea1aa3 -[RCTHTTPRequestHandler URLSession:task:didCompleteWithError:] + 403 10 CFNetwork 0x000000010cf3a437 __51-[NSURLSession delegate_task:didCompleteWithError:]_block_invoke.207 + 80 11 Foundation 0x000000010b885363 __NSBLOCKOPERATION_IS_CALLING_OUT_TO_A_BLOCK__ + 7 12 Foundation 0x000000010b8851ca -[NSBlockOperation main] + 68 13 Foundation 0x000000010b8836b2 -[__NSOperationInternal _start:] + 766 14 libdispatch.dylib 0x0000000112457779 _dispatch_client_callout + 8 15 libdispatch.dylib 0x000000011245c931 _dispatch_block_invoke_direct + 317 16 libdispatch.dylib 0x0000000112457779 _dispatch_client_callout + 8 17 libdispatch.dylib 0x000000011245c931 _dispatch_block_invoke_direct + 317 18 libdispatch.dylib 0x000000011245c7d4 dispatch_block_perform + 109 19 Foundation 0x000000010b87f75b __NSOQSchedule_f + 337 20 libdispatch.dylib 0x0000000112457779 _dispatch_client_callout + 8 21 libdispatch.dylib 0x000000011245f1b2 _dispatch_queue_serial_drain + 735 22 libdispatch.dylib 0x000000011245f9af _dispatch_queue_invoke + 321 23 libdispatch.dylib 0x0000000112461cf8 _dispatch_root_queue_drain + 473 24 libdispatch.dylib 0x0000000112461ac1 _dispatch_worker_thread3 + 119 25 libsystem_pthread.dylib 0x000000011297a169 _pthread_wqthread + 1387 26 libsystem_pthread.dylib 0x0000000112979be9 start_wqthread + 13 ) libc++abi.dylib: terminating with uncaught exception of type NSException ``` Bug can be reproduced by making a `XMLHttpRequest` (uses `RCTNetworking` internally) that takes a couple seconds to perform, and issuing a RCTBridge reload command in the meantime. You can add the following code to the react-native template project, ``` componentDidMount() { var oReq = new XMLHttpRequest(); oReq.addEventListener("load", () => console.log('Finished')); oReq.open("GET", "https://www.dropbox.com/s/o01hz0chqvjafhv/file.bin?dl=1"); oReq.send(); console.log('Request is being performed...') } ``` In my case I download a 1MB file. Run the project and reload the a couple times. Bug is triggered. [INTERNAL] [BUGFIX] [RCTNetworking] - Clear handlers and tasks on RCTNetworking invalidation Closes facebook#19169 Differential Revision: D8053070 Pulled By: hramos fbshipit-source-id: d8af54fecd99173905363f962ffc638ef8b85082
Summary: This PR fixes a bug where in `RCTNetworking` not all tasks/handlers were not being cleared when invalidating the class. I came across this issue when writing some unit tests for my native plugins, sometimes a test would finish running (and the bridge invalidated), and only afterwards a callback from RCTNetworking would come, resulting in this exception: ``` 2018-05-07 15:23:34.264494-0700 Guardian[73794:10710945] *** Assertion failure in -[RCTEventEmitter sendEventWithName:body:](), /Users/.../app/node_modules/react-native/React/Modules/RCTEventEmitter.m:41 2018-05-07 15:23:34.276505-0700 Guardian[73794:10710945] *** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'Error when sending event: didCompleteNetworkResponse with body: ( 2, cancelled, 0 ). Bridge is not set. This is probably because you've explicitly synthesized the bridge in RCTNetworking, even though it's inherited from RCTEventEmitter.' *** First throw call stack: ( 0 CoreFoundation 0x000000010d5b21e6 __exceptionPreprocess + 294 1 libobjc.A.dylib 0x000000010be6f031 objc_exception_throw + 48 2 CoreFoundation 0x000000010d5b7472 +[NSException raise:format:arguments:] + 98 3 Foundation 0x000000010b94864f -[NSAssertionHandler handleFailureInFunction:file:lineNumber:description:] + 165 4 Guardian 0x0000000106ff5227 -[RCTEventEmitter sendEventWithName:body:] + 567 5 Guardian 0x0000000106e9ebab __76-[RCTNetworking sendRequest:responseType:incrementalUpdates:responseSender:]_block_invoke.423 + 1115 6 Guardian 0x0000000106e8f48c __50-[RCTNetworkTask URLRequest:didCompleteWithError:]_block_invoke + 92 7 Guardian 0x0000000106e8ded1 -[RCTNetworkTask dispatchCallback:] + 113 8 Guardian 0x0000000106e8f37a -[RCTNetworkTask URLRequest:didCompleteWithError:] + 410 9 Guardian 0x0000000106ea1aa3 -[RCTHTTPRequestHandler URLSession:task:didCompleteWithError:] + 403 10 CFNetwork 0x000000010cf3a437 __51-[NSURLSession delegate_task:didCompleteWithError:]_block_invoke.207 + 80 11 Foundation 0x000000010b885363 __NSBLOCKOPERATION_IS_CALLING_OUT_TO_A_BLOCK__ + 7 12 Foundation 0x000000010b8851ca -[NSBlockOperation main] + 68 13 Foundation 0x000000010b8836b2 -[__NSOperationInternal _start:] + 766 14 libdispatch.dylib 0x0000000112457779 _dispatch_client_callout + 8 15 libdispatch.dylib 0x000000011245c931 _dispatch_block_invoke_direct + 317 16 libdispatch.dylib 0x0000000112457779 _dispatch_client_callout + 8 17 libdispatch.dylib 0x000000011245c931 _dispatch_block_invoke_direct + 317 18 libdispatch.dylib 0x000000011245c7d4 dispatch_block_perform + 109 19 Foundation 0x000000010b87f75b __NSOQSchedule_f + 337 20 libdispatch.dylib 0x0000000112457779 _dispatch_client_callout + 8 21 libdispatch.dylib 0x000000011245f1b2 _dispatch_queue_serial_drain + 735 22 libdispatch.dylib 0x000000011245f9af _dispatch_queue_invoke + 321 23 libdispatch.dylib 0x0000000112461cf8 _dispatch_root_queue_drain + 473 24 libdispatch.dylib 0x0000000112461ac1 _dispatch_worker_thread3 + 119 25 libsystem_pthread.dylib 0x000000011297a169 _pthread_wqthread + 1387 26 libsystem_pthread.dylib 0x0000000112979be9 start_wqthread + 13 ) libc++abi.dylib: terminating with uncaught exception of type NSException ``` Bug can be reproduced by making a `XMLHttpRequest` (uses `RCTNetworking` internally) that takes a couple seconds to perform, and issuing a RCTBridge reload command in the meantime. You can add the following code to the react-native template project, ``` componentDidMount() { var oReq = new XMLHttpRequest(); oReq.addEventListener("load", () => console.log('Finished')); oReq.open("GET", "https://www.dropbox.com/s/o01hz0chqvjafhv/file.bin?dl=1"); oReq.send(); console.log('Request is being performed...') } ``` In my case I download a 1MB file. Run the project and reload the a couple times. Bug is triggered. [INTERNAL] [BUGFIX] [RCTNetworking] - Clear handlers and tasks on RCTNetworking invalidation Closes facebook/react-native#19169 Differential Revision: D8053070 Pulled By: hramos fbshipit-source-id: d8af54fecd99173905363f962ffc638ef8b85082
This PR fixes a bug where in
RCTNetworking
not all tasks/handlers were not being cleared when invalidating the class.I came across this issue when writing some unit tests for my native plugins, sometimes a test would finish running (and the bridge invalidated), and only afterwards a callback from RCTNetworking would come, resulting in this exception:
Test Plan
Bug can be reproduced by making a
XMLHttpRequest
(usesRCTNetworking
internally) that takes a couple seconds to perform, and issuing a RCTBridge reload command in the meantime.You can add the following code to the react-native template project,
In my case I download a 1MB file.
Run the project and reload the a couple times. Bug is triggered.
Release Notes
[INTERNAL] [BUGFIX] [RCTNetworking] - Clear handlers and tasks on RCTNetworking invalidation