From d352e2c837a5f566f139f52f8d473f8987ef80f0 Mon Sep 17 00:00:00 2001 From: Deepu Ganapathiyadan Date: Mon, 6 Jan 2020 14:17:39 -0800 Subject: [PATCH] Fix request handler unregistration issue Add more tests. The old implementation was unregistering wrong request handlers due to a bad UUID mapping Now, when a new request handler registration comes in the old one is automatically removed before registering the new one. --- .../bridge/RequestHandleTests.java | 80 +++++++++++++++++++ .../bridge/ElectrodeBridgeTransceiver.java | 2 +- .../bridge/ElectrodeNativeBridge.java | 3 +- .../bridge/RequestHandlerProcessor.java | 15 ++-- .../reactnative/bridge/RequestRegistrar.java | 2 +- .../bridge/RequestRegistrarImpl.java | 23 +++++- 6 files changed, 110 insertions(+), 15 deletions(-) diff --git a/android/electrode-reactnative-bridge/src/androidTest/java/com/walmartlabs/electrode/reactnative/bridge/RequestHandleTests.java b/android/electrode-reactnative-bridge/src/androidTest/java/com/walmartlabs/electrode/reactnative/bridge/RequestHandleTests.java index d94f33bd..b755eada 100644 --- a/android/electrode-reactnative-bridge/src/androidTest/java/com/walmartlabs/electrode/reactnative/bridge/RequestHandleTests.java +++ b/android/electrode-reactnative-bridge/src/androidTest/java/com/walmartlabs/electrode/reactnative/bridge/RequestHandleTests.java @@ -105,4 +105,84 @@ public void onFailure(@NonNull FailureMessage failureMessage) { }); waitForCountDownToFinishOrFail(countDownLatch); } + + @Test + public void testMultipleRequestHandlers() { + final CountDownLatch countDownLatch = new CountDownLatch(4); + final int RESULT_AGE_FIRST = 10; + final int RESULT_AGE_SECOND = 20; + final int RESULT_AGE_THIRD = 30; + + RequestHandlerHandle handlerHandle = PersonApi.requests().registerGetAgeRequestHandler(new ElectrodeBridgeRequestHandler() { + @Override + public void onRequest(@Nullable String payload, @NonNull ElectrodeBridgeResponseListener responseListener) { + responseListener.onSuccess(RESULT_AGE_FIRST); + countDownLatch.countDown(); + } + }); + + RequestHandlerHandle handlerHandle1 = PersonApi.requests().registerGetAgeRequestHandler(new ElectrodeBridgeRequestHandler() { + @Override + public void onRequest(@Nullable String payload, @NonNull ElectrodeBridgeResponseListener responseListener) { + responseListener.onSuccess(RESULT_AGE_SECOND); + countDownLatch.countDown(); + } + }); + + RequestHandlerHandle handlerHandle2 = PersonApi.requests().registerGetAgeRequestHandler(new ElectrodeBridgeRequestHandler() { + @Override + public void onRequest(@Nullable String payload, @NonNull ElectrodeBridgeResponseListener responseListener) { + responseListener.onSuccess(RESULT_AGE_THIRD); + countDownLatch.countDown(); + } + }); + + PersonApi.requests().getAge("user", new ElectrodeBridgeResponseListener() { + @Override + public void onSuccess(@Nullable Integer responseData) { + assertNotNull(responseData); + assertEquals(RESULT_AGE_THIRD, responseData.intValue()); + countDownLatch.countDown(); + } + + @Override + public void onFailure(@NonNull FailureMessage failureMessage) { + fail(); + } + }); + + handlerHandle.unregister(); + handlerHandle1.unregister(); + + PersonApi.requests().getAge("user", new ElectrodeBridgeResponseListener() { + @Override + public void onSuccess(@Nullable Integer responseData) { + assertNotNull(responseData); + assertEquals(RESULT_AGE_THIRD, responseData.intValue()); + countDownLatch.countDown(); + } + + @Override + public void onFailure(@NonNull FailureMessage failureMessage) { + fail(); + } + }); + + handlerHandle2.unregister(); + + PersonApi.requests().getAge("user", new ElectrodeBridgeResponseListener() { + @Override + public void onSuccess(@Nullable Integer responseData) { + fail(); + } + + @Override + public void onFailure(@NonNull FailureMessage failureMessage) { + assertNotNull(failureMessage); + countDownLatch.countDown(); + } + }); + + waitForCountDownToFinishOrFail(countDownLatch); + } } diff --git a/android/electrode-reactnative-bridge/src/main/java/com/walmartlabs/electrode/reactnative/bridge/ElectrodeBridgeTransceiver.java b/android/electrode-reactnative-bridge/src/main/java/com/walmartlabs/electrode/reactnative/bridge/ElectrodeBridgeTransceiver.java index 0d3f5952..c9fad9b6 100644 --- a/android/electrode-reactnative-bridge/src/main/java/com/walmartlabs/electrode/reactnative/bridge/ElectrodeBridgeTransceiver.java +++ b/android/electrode-reactnative-bridge/src/main/java/com/walmartlabs/electrode/reactnative/bridge/ElectrodeBridgeTransceiver.java @@ -146,7 +146,7 @@ public static void addConstantsProvider(@NonNull ConstantsProvider constantsProv sConstantsProviders.add(constantsProvider); } - @NonNull + @Nullable @Override public UUID getRequestHandlerId(@NonNull String name) { return sRequestRegistrar.getRequestHandlerId(name); diff --git a/android/electrode-reactnative-bridge/src/main/java/com/walmartlabs/electrode/reactnative/bridge/ElectrodeNativeBridge.java b/android/electrode-reactnative-bridge/src/main/java/com/walmartlabs/electrode/reactnative/bridge/ElectrodeNativeBridge.java index 0d838605..b9a01184 100644 --- a/android/electrode-reactnative-bridge/src/main/java/com/walmartlabs/electrode/reactnative/bridge/ElectrodeNativeBridge.java +++ b/android/electrode-reactnative-bridge/src/main/java/com/walmartlabs/electrode/reactnative/bridge/ElectrodeNativeBridge.java @@ -17,6 +17,7 @@ package com.walmartlabs.electrode.reactnative.bridge; import android.support.annotation.NonNull; +import android.support.annotation.Nullable; import java.util.UUID; @@ -66,7 +67,7 @@ public interface ElectrodeNativeBridge { * @param name * @return {@link UUID} of the request handler */ - @NonNull + @Nullable UUID getRequestHandlerId(@NonNull String name); /** diff --git a/android/electrode-reactnative-bridge/src/main/java/com/walmartlabs/electrode/reactnative/bridge/RequestHandlerProcessor.java b/android/electrode-reactnative-bridge/src/main/java/com/walmartlabs/electrode/reactnative/bridge/RequestHandlerProcessor.java index 51540952..eb8aebd3 100644 --- a/android/electrode-reactnative-bridge/src/main/java/com/walmartlabs/electrode/reactnative/bridge/RequestHandlerProcessor.java +++ b/android/electrode-reactnative-bridge/src/main/java/com/walmartlabs/electrode/reactnative/bridge/RequestHandlerProcessor.java @@ -91,15 +91,10 @@ public void onSuccess(TResp obj) { @Override public boolean unregister() { - Logger.d(TAG, "Removing registered request handler %s with id %s", handler, id); - ElectrodeBridgeRequestHandler removedHandler = ElectrodeBridgeHolder.unregisterRequestHandler(id); - if (handler != null && intermediateRequestHandler == removedHandler) { - intermediateRequestHandler = null; - handler = null; - return true; - } else { - Logger.w(TAG, "Not able to unregister a request handler. This is not normal as the request handle should have proper reference of the registered handler. "); - } - return false; + Logger.w(TAG, "Removing registered request handler %s with id %s", handler, id); + ElectrodeBridgeHolder.unregisterRequestHandler(id); + intermediateRequestHandler = null; + handler = null; + return true; } } diff --git a/android/electrode-reactnative-bridge/src/main/java/com/walmartlabs/electrode/reactnative/bridge/RequestRegistrar.java b/android/electrode-reactnative-bridge/src/main/java/com/walmartlabs/electrode/reactnative/bridge/RequestRegistrar.java index a58dd99d..9d3cab7f 100644 --- a/android/electrode-reactnative-bridge/src/main/java/com/walmartlabs/electrode/reactnative/bridge/RequestRegistrar.java +++ b/android/electrode-reactnative-bridge/src/main/java/com/walmartlabs/electrode/reactnative/bridge/RequestRegistrar.java @@ -59,6 +59,6 @@ public interface RequestRegistrar { * @param name * @return {@link UUID} of the request handler */ - @NonNull + @Nullable UUID getRequestHandlerId(@NonNull String name); } diff --git a/android/electrode-reactnative-bridge/src/main/java/com/walmartlabs/electrode/reactnative/bridge/RequestRegistrarImpl.java b/android/electrode-reactnative-bridge/src/main/java/com/walmartlabs/electrode/reactnative/bridge/RequestRegistrarImpl.java index 1a074129..0484ff17 100644 --- a/android/electrode-reactnative-bridge/src/main/java/com/walmartlabs/electrode/reactnative/bridge/RequestRegistrarImpl.java +++ b/android/electrode-reactnative-bridge/src/main/java/com/walmartlabs/electrode/reactnative/bridge/RequestRegistrarImpl.java @@ -20,11 +20,14 @@ import android.support.annotation.Nullable; import android.support.annotation.VisibleForTesting; +import com.walmartlabs.electrode.reactnative.bridge.helpers.Logger; + import java.util.Map; import java.util.UUID; import java.util.concurrent.ConcurrentHashMap; public class RequestRegistrarImpl implements RequestRegistrar { + private static final String TAG = RequestRegistrarImpl.class.getSimpleName(); private final ConcurrentHashMap mRequestNameByUUID = new ConcurrentHashMap<>(); private final ConcurrentHashMap mRequestHandlerByRequestName = new ConcurrentHashMap<>(); @@ -40,9 +43,14 @@ public class RequestRegistrarImpl implements RequestRegistrar { @NonNull public boolean registerRequestHandler(@NonNull String name, @NonNull T requestHandler, @NonNull UUID requestHandlerUuid) { boolean isRegistered; + if (mRequestHandlerByRequestName.get(name) != null) { + Logger.d(TAG, "A request handler for request(name: %s) already exist. Replacing with a new request handler", name); + removeOldUuidMapping(name); + } mRequestHandlerByRequestName.put(name, requestHandler); mRequestNameByUUID.put(requestHandlerUuid, name); isRegistered = true; + Logger.d(TAG, "New request handler(id: %s) registered for request: %s", requestHandlerUuid, name); return isRegistered; } @@ -55,7 +63,10 @@ public boolean registerRequestHandler(@NonNull String name, @NonNull T requestHa public T unregisterRequestHandler(@NonNull UUID requestHandlerUuid) { String requestName = mRequestNameByUUID.remove(requestHandlerUuid); if (requestName != null) { + Logger.d(TAG, "Request handler(id: %s) removed for request: %s", requestHandlerUuid, requestName); return mRequestHandlerByRequestName.remove(requestName); + } else { + Logger.d(TAG, "Request handler(id: %s) already removed", requestHandlerUuid); } return null; } @@ -72,17 +83,25 @@ public T getRequestHandler(@NonNull String name) { return mRequestHandlerByRequestName.get(name); } - @NonNull + @Nullable @Override public UUID getRequestHandlerId(@NonNull String name) { for (Map.Entry entry : mRequestNameByUUID.entrySet()) { - if (name != null && name.equals(entry.getValue())) { + if (name.equals(entry.getValue())) { return (UUID) entry.getKey(); } } return null; } + private void removeOldUuidMapping(@NonNull String name) { + UUID oldUuid = getRequestHandlerId(name); + if (oldUuid != null) { + Logger.d(TAG, "Removing old request handler(id: %s)", oldUuid); + mRequestNameByUUID.remove(oldUuid); + } + } + /** * ONLY FOR TESTING *