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

Generate unique identifier for each TranserUtility instance upon setup #27

Closed
wants to merge 1 commit into from

Conversation

EranSch
Copy link

@EranSch EranSch commented Dec 7, 2016

First off, I want to thank you for your work on this package. This thing runs like a boss! Very cool!

Problem

In development and testing, we often encountered the issue described in #9 but brushed it off in light of the fact that it seemed to be more of a development pain than something critical. Once our iOS implementation went into QA, however, the issue came back into question for the following reason:

The basic credentials we use to authenticate TransferUtility have a one hour expiration. As such, after an hour has elapsed, we must update the credentials originally used to setup the package. Once setupWithBasic is invoked a second time, a new TransferUtility instance would be created using the same static key and then subscriptions would no longer bubble up to the JavaScript module.

I should note that ths is not a problem on Android.

I spent yesterday trying to determine a sound resolution to this issue but nothing obvious came together (perhaps a result of my lack of experience with both Obj-C and the AWS SDK on iOS). I was able to determine that simply unregistering the TransferUtility instance upon subsequent setups seemed to be problematic.

Proposed Solution

Largely in the interest of getting something working, I came came up with the revision I present in this PR. The basic change is pretty simple:

  1. Upon setup, generate a unique string and store it as an instance property
  2. Use the unique identifier as:
  • The key for the TransferUtility instance
  • A prepended component of each task ID
  1. Upon a later setup, create a new unique string to use for the next TransferUtility instance

Additionally the logic surrounding the alreadyInitialize property was removed as it is no longer necessary, as the full setup process is expected to run regardless.

Potential Concerns

My concern with the current codebase is that old TransferUtility instances are not currently handled.

My first thought was memory leak however after doing some local testing and repeatedly creating new instances, I didn't experience any severe impacts in terms of RAM consumption or stability... needless to say, it still feels sloppy.

I'd be open to adjustments in cleaning this aspect up if the rest of the implementation seems reasonable.

Looking forward to hearing back. Thanks again for your stellar work on this!

Eran

@EranSch
Copy link
Author

EranSch commented Dec 7, 2016

Looks like the build is breaking due to a peer dependency issue regarding React Native <> React <> React Native Mock versions...

npm ERR! Linux 4.8.7-040807-generic
npm ERR! argv "/home/travis/.nvm/versions/node/v4.7.0/bin/node" "/home/travis/.nvm/versions/node/v4.7.0/bin/npm" "install"
npm ERR! node v4.7.0
npm ERR! npm  v2.15.11
npm ERR! code EPEERINVALID

npm ERR! peerinvalid The package [email protected] does not satisfy its siblings' peerDependencies requirements!
npm ERR! peerinvalid Peer [email protected] wants react@*
npm ERR! peerinvalid Peer [email protected] wants react@~15.3.1

npm ERR! Please include the following file with any support request:
npm ERR!     /home/travis/build/mybigday/react-native-s3/npm-debug.log


The command "npm install" failed and exited with 1 during .

Your build has been stopped.

I'm happy to make adjustments to this but it's a bit out of the scope of the PR so I'll hold off unless someone requests it specifically.

@jhen0409
Copy link
Member

jhen0409 commented Dec 9, 2016

Thanks! Also sorry for delay..

I expected it's support background task (persist the same TransferUtility instance), so it should use the same key, otherwise it will loose all running tasks.

The following code will fix #9 I was tried:

AWSS3TransferUtility *transferUtility = [AWSS3TransferUtility S3TransferUtilityForKey:@"RNS3TransferUtility"];

if (!transferUtility) {
  [AWSS3TransferUtility registerS3TransferUtilityWithConfiguration:configuration
                                                            forKey:@"RNS3TransferUtility"];
}

UPDATE: It should broken changed config, so we need remember the last configuration, or provide rememberLastInstance option on JS layer, to avoid it break the background tasks.

For your problem, I think we should support multi-instance for TransferUtility (#10), It may have more extra work. What do you think?

My concern with the current codebase is that old TransferUtility instances are not currently handled.

A simple workaround may following code:

if (_transferUtilityKey) {
  [AWSS3TransferUtility removeS3TransferUtilityForKey:_transferUtilityKey];
}
_transferUtilityKey = [[NSProcessInfo processInfo] globallyUniqueString];

@EranSch
Copy link
Author

EranSch commented Dec 19, 2016

No worries on the delay, as you can tell, I too have limited time 😄

That seems much more straightforward than my approach. Just goes to show you what being familiar with AWS-SDK and Obj-C can do for you! Your proposed change makes sense to me and seems like it would work correctly however I'm unclear of the issue you describe in your edit. Would you mind elaborating?

I attempted your second example which would theoretically clean up old instances however this immediately suspends all currently running transfers. I think the logic would ideally need to be something more elaborate that would remove old instances only after all transfers are complete... would you agree?

Thanks!

@EranSch EranSch closed this Jan 15, 2017
@EranSch EranSch deleted the allow-multiple-setups branch January 15, 2017 12:33
@EranSch EranSch restored the allow-multiple-setups branch January 15, 2017 12:33
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.

2 participants