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

Synchronized #7

Closed
wants to merge 10 commits into from
Closed

Synchronized #7

wants to merge 10 commits into from

Conversation

chrusart
Copy link

COPY OF: wkh237#636

Respond if I should still make it to 0.10.9 branch.

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:

  • keeping tasks references to cancel them, change their progress config etc.,
  • session and requests logic.

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 log NSLog(@"object released by ARC."); that it should or was used), anyway, it's now instantiated with NSMapTableWeakMemory 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
  • __block storage type not needed, variables not used in blocks in scope
  • extern storage type not used in .m files
  • nullability type in RNFetchBlobNetwork, should be checked and applied for method arguments in other classes
  • unimplemented + (void) enableProgressReport:(NSString *) taskId, + (void) enableUploadProgress:(NSString *) taskId, - (void) sendRequest method declarations removed
    unused fileTaskCompletionHandler and dataTaskCompletionHandler properties and synthesizers removed
  • other small warning messages fixes
  • every single request is created on new RNFetchBlobNetwork instance so taskQueue.maxConcurrentOperationCount seems not work as expected -> made taskQueue instantiated per RNFetchBlobNetwork instance.

I 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 (except defaultSessionConfiguration)? 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:

  • subclass RCTEventEmitter to send events
  • expired task logic?

This is WIP, 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

DOES NOT HAVE wkh237:0.11.0 commits listed in this PR!

feel free to:
"react-native-fetch-blob": "github:flatfox-ag/react-native-fetch-blob#exception_fixes",

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 :)

@chrusart
Copy link
Author

chrusart commented Mar 27, 2018

For now no crashes from this branch and from libraries that use fetch-blob on our production.

@chrusart chrusart closed this Mar 27, 2018
@chrusart
Copy link
Author

WROND BRANCH

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant