-
Notifications
You must be signed in to change notification settings - Fork 785
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
Synchronized tables, separated requests and network logic and other... #8
Conversation
remove extra space
…o get sd card directories. sd card directiories as constants deprecated warning
We have a crash in our application, and based on our analysis the culprit is a race condition in RNFetchBlobNetwork initialization. Crashing thread: Crashed: com.apple.root.default-qos 0 libobjc.A.dylib 0x10416bacb objc_msgSend + 11 1 Foundation 0x103d3b644 -[NSObject(NSKeyValueObservingPrivate) _changeValueForKeys:count:maybeOldValuesDict:usingBlock:] + 153 2 Foundation 0x103c23f8c -[NSObject(NSKeyValueObservingPrivate) _changeValueForKey:key:key:usingBlock:] + 61 3 Foundation 0x103c23f3a -[NSOperationQueue setMaxConcurrentOperationCount:] + 198 4 prymr 0x100d579aa -[RNFetchBlobNetwork init] (RNFetchBlobNetwork.m:112) 5 prymr 0x100d63a73 __65-[RNFetchBlob fetchBlob:taskId:method:url:headers:body:callback:]_block_invoke (RNFetchBlob.m:131) 6 prymr 0x100d6006b __85+[RNFetchBlobReqBuilder buildOctetRequest:taskId:method:url:headers:body:onComplete:]_block_invoke (RNFetchBlobReqBuilder.m:178) 7 libdispatch.dylib 0x107511585 _dispatch_call_block_and_release + 12 While a second thread is running: com.apple.root.default-qos 0 libsystem_kernel.dylib 0x107891c22 __psynch_mutexwait + 10 1 libsystem_pthread.dylib 0x1078c6dfa _pthread_mutex_lock_wait + 100 2 libsystem_pthread.dylib 0x1078c4519 _pthread_mutex_lock_slow + 285 3 Foundation 0x103d3b615 -[NSObject(NSKeyValueObservingPrivate) _changeValueForKeys:count:maybeOldValuesDict:usingBlock:] + 106 4 Foundation 0x103c23f8c -[NSObject(NSKeyValueObservingPrivate) _changeValueForKey:key:key:usingBlock:] + 61 5 Foundation 0x103c23f3a -[NSOperationQueue setMaxConcurrentOperationCount:] + 198 6 prymr 0x100d579aa -[RNFetchBlobNetwork init] (RNFetchBlobNetwork.m:112) 7 prymr 0x100d63a73 __65-[RNFetchBlob fetchBlob:taskId:method:url:headers:body:callback:]_block_invoke (RNFetchBlob.m:131) 8 prymr 0x100d6006b __85+[RNFetchBlobReqBuilder buildOctetRequest:taskId:method:url:headers:body:onComplete:]_block_invoke (RNFetchBlobReqBuilder.m:178) 9 libdispatch.dylib 0x107511585 _dispatch_call_block_and_release + 12 The patch just adds a dumb double-synchronization to the initialization.
For now no crashes from this branch and from libraries that use fetch-blob on our production. |
Got some after merge errors with promises, working on that |
…fetch-blob into exception_fixes
Looks good now, but HEAD not compatible anymore with 0.10.8 because interfaces changed in 0.10.9 from callbacks to promises and if someone used our branch to fix exceptions then use: P.S. anyway, always use commit hash instead branch name, cause it can be changed by owner. |
@chrusart Thank you for this PR, it looks like you've put a lot of work in and you've even tested that it resolved issues for you in production! Am I correct in assuming that these are mostly internal changes and that there aren't any changes in the public API of the library with this PR? |
Hi @Traviskn , the 0.10.9 branch have changes that uses Promises instead callbacks, and that was done before, not by me. The problem is when somebody will use 'custom' version (branch) in his projects package.json. In that case native compiled code will be taken from this one, but if library has different version (react-native-cached-image for example has 0.10.8) then packager will download this version specially for this library and it will be in node_modules of this library. What happens then is that library will use his own react-native-fetch-blob javascript code which uses callbacks in 0.10.8, but native compiled code will be from version you have in your projects package.json. So, for example mkdir in fs.js in 0.10.9:
(other: and it will crash the app if native code uses promises, but js callbacks. And coming back to your question about interface. I hope it's clear enough and I understood and tested correctly to get to this conclusions. |
@chrusart Thanks for the overview! |
COPY OF: wkh237#636
Hi,
this PR came from crashes we still get (some description):
wcandillon/react-native-img-cache#95
...but evolved.
Whole RNFetchBlobNetwork class actually do two things:
I moved session and requests logic into separate file, now it's more clean what do what.
NSMapTable
seemed to be used incorrectly or just it's (weak) features not used (it seems from logNSLog(@"object released by ARC.");
that it should or was used), anyway, it's now instantiated withNSMapTableWeakMemory
option for objects, so requests will be released, default in NSMapTable is to use strong references.sharedInstance
is used instead tables and dictionaries instances created on load. This pattern is used all around but please give me a feedback why previous is better solution, maybe I missed something.Other things done in this PR when implementing:
checkExpiredSessions
didn’t find anywhere, removed from header+ (void) enableProgressReport:(NSString *) taskId, + (void) enableUploadProgress:(NSString *) taskId, - (void) sendRequest
method declarations removedunused
fileTaskCompletionHandler
anddataTaskCompletionHandler
properties and synthesizers removedI can't find any place where actually something is added to
expirationTable
.Does
HTTPMaximumConnectionsPerHost = 10
have any impact as new session is created for every request (exceptdefaultSessionConfiguration
)? In docs it says that it applies to all sessions'...based on this configuration'
, what actually this means, the same instance, identifier or some comparison???I'm aware of few fixes that are in 0.10.9 not included here.
TODO:
This works for now for us, but didn't test everything and any feedback or testing would be great.
POST 16.01.2018
What about applying:
http://google.github.io/styleguide/objcguide.html
I agree with that code guideline mostly.
POST 19.01.2018:
https://github.com/flatfox-ag/react-native-fetch-blob/tree/exception_fixes 2 days already in production, no exceptions yet.
This branch have all these fixes:
wkh237#619
wkh237#499
and all changes from:
https://github.com/flatfox-ag/react-native-fetch-blob/tree/synchronized
feel free to:
"react-native-fetch-blob": "github:flatfox-ag/react-native-fetch-blob#a46bf8180d76131eafe84ab44e0436322073820c",
As we use it in production I removed WIP note.
Stuff in TODO will come in different PRs I guess.
POST 25.01.2018
Week without crashes on production, fixes confirmed :)