fix connection issue

[ianlin-bbpos]

Summary

fix connection issues with beta.20, improve the method of fetching and setting token.

Motivation

To fix the issues reported by users that connecting device would timeout on the first running after app installed.

Testing

• I tested this manually
• I added automated tests

Documentation

Select one:

• I have added relevant documentation for my changes.
• This PR does not result in any developer-facing changes.

[ebarrenechea-stripe]

Could you explain the problem a bit more? I think I'm missing a bit of context here. Are we currently overwriting the ConnectionTokenCallback instance when receiving more than one request? Is the ConnectionTokenCallback implemented in native code (I couldn't find anything in the repo) or is it a react native component?

[ebarrenechea-stripe]

We should remove this instance property and make the callback a locally declared variable in setConnectionToken below.

[ebarrenechea-stripe]

I'm a bit worried that we add a ConnectionTokenCallback instance to the map and never remove it. I imagine it's most likely the same instance over and over again but there's nothing that guarantees that. We could be causing a memory leak here.

[ianlin-bbpos]

You are absolutely right. I didn't think enough about this situation. Do you think it's enough if I add a remove of the callback after calling once, since that every request of fectchConnectionToken seems there must be a calling of setConnectionToken

[ebarrenechea-stripe]

Yes, it will be fine if we remove it once we're done.

[ianlin-bbpos]

Could you explain the problem a bit more? I think I'm missing a bit of context here. Are we currently overwriting the ConnectionTokenCallback instance when receiving more than one request? Is the ConnectionTokenCallback implemented in native code (I couldn't find anything in the repo) or is it a react native component?

yes, user reported that the connecting the device on the first running of the app after installation would timeout, and connect successful after reopen the app. and we have some findings here https://docs.google.com/document/d/1PMfnDrshVFXo3H2WkPITNvGwMwkme0irJ482heOeq4w
now we are looking for a solution to resolve it. In this PR, we tried to use a HashMap to record the callback via a uuid as the keys on each intent to fetch token. then we can identify the callback for each request, to avoid that callback would be covered by later request. ConnectionTokenCallback is a Terminal native component, it's impletemented in native sdk part

[tim-lin-bbpos]

Nit: Maybe we can extract all the below duplicate call to init and remove here.

[ebarrenechea-stripe]

I understand the issue better now, thanks for the doc. If this code is being called from multiple threads it might be better to use a HashTable instead of a HashMap as the former is thread safe. I wonder if we could re-use the first token instead of fetching a new one by synchronizing the requests. In any case, this is good enough for now.

Changes look good to me but I'll leave the final approval with @bric-stripe to double check the iOS side.

[bric-stripe]

requesting changes to provide some logging if we can't map to a completion

ideally we wouldn't need the integration to deal with the callbackId. But I understand how things are now where setConnectionToken is a separate method so we need some way to connect the dots to call back in to the native SDK

[bric-stripe]

since this function doesn't do anything if the callback id is nil or it doesn't have a matching completion, I suggest using a single check at the top that logs a message and early returns if things aren't as they should be:

guard let callbackId,
     let completionCallback = self.callbackMap[callbackId] else { 
    print("⚠️ setConnectionToken requires the callbackId be set to the callbackId value provided to the tokenProviderHandler.")
    return
}

otherwise it would fail silently which would likely be tricky for people to debug.

and something similar on the Android side

[ianlin-bbpos]

ok, let me improve it.

[ianlin-bbpos]

I understand the issue better now, thanks for the doc. If this code is being called from multiple threads it might be better to use a HashTable instead of a HashMap as the former is thread safe. I wonder if we could re-use the first token instead of fetching a new one by synchronizing the requests. In any case, this is good enough for now.

Changes look good to me but I'll leave the final approval with @bric-stripe to double check the iOS side.

Thanks, let me change to use a HashTable.

[tim-lin-bbpos]

I understand the issue better now, thanks for the doc. If this code is being called from multiple threads it might be better to use a HashTable instead of a HashMap as the former is thread safe. I wonder if we could re-use the first token instead of fetching a new one by synchronizing the requests. In any case, this is good enough for now.

Changes look good to me but I'll leave the final approval with @bric-stripe to double check the iOS side.

Q: This might be off-topic and thread safe is definitely a good thing if we can't control the upstream, but I still like to clarify on the usage of HashTable. If the only purpose is thread safe, are we gonna consider to use ConcurrentHashMap instead? According to the HashTable javadoc, it should be a better option as far as I know.
p.s. definitely we can go with HashTable for now as it will only called a few times here (and should only one in the future?).

Ref: https://docs.oracle.com/javase/8/docs/api/java/util/Hashtable.html

As of the Java 2 platform v1.2, this class was retrofitted to implement the Map interface, making it a member of the Java Collections Framework. Unlike the new collection implementations, Hashtable is synchronized. If a thread-safe implementation is not needed, it is recommended to use HashMap in place of Hashtable. If a thread-safe highly-concurrent implementation is desired, then it is recommended to use ConcurrentHashMap in place of Hashtable.

[ebarrenechea-stripe]

Yeah, ConcurrentHashMap will work as well. We just need a data structure that can handle concurrency without issues.

feedback addressed

[maestrodrew]

@nazli-stripe When can we expect the next release (v21) that will contain this fix?