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.

Sign up for GitHub

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

Fix request handle un registration #63

Merged
merged 2 commits into from
Jan 7, 2020
Merged

Fix request handle un registration #63

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
65d1bfc
Move RequestHandle test cases to separate class
deepueg Jan 6, 2020
d352e2c
Fix request handler unregistration issue
deepueg Jan 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
188 changes: 188 additions & 0 deletions ...src/androidTest/java/com/walmartlabs/electrode/reactnative/bridge/RequestHandleTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,188 @@
package com.walmartlabs.electrode.reactnative.bridge;

import android.support.annotation.NonNull;
import android.support.annotation.Nullable;

import com.walmartlabs.electrode.reactnative.sample.api.PersonApi;

import org.junit.Test;

import java.util.concurrent.CountDownLatch;

import static junit.framework.TestCase.assertEquals;
import static junit.framework.TestCase.assertNotNull;
import static junit.framework.TestCase.assertSame;
import static junit.framework.TestCase.fail;

public class RequestHandleTests extends BaseBridgeTestCase {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the class name be RequestHandlerTests?

Copy link
Contributor Author

@deepueg deepueg Jan 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one!

Just confirming and double checking a couple of things:

  • There can only be one registered handler for the request/response functionality (unlike "events", which can have multiple handlers). Is that assumption correct?
  • Previously, what would happen if multiple handlers were registered for request/response? I guess we did not have a test case for that..

  • Yes, we can only have one request handler at any given point of time.

  • Previously, the mRequestNameByUUID map was not updated when a new request handler is registered. Due to this, when someone calls unregisterRequestHandler on an old UUID the implementation was removing the current one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RequestHandle is the return type that stores the ID, hence kept this name. Guess it can be either.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.. Just keep it. We can consider some naming refactoring/simplification for v2.


@Test
public void testRequestHandlerRemoval() {
final CountDownLatch countDownLatch = new CountDownLatch(3);
final String requestName = "testRequestHandlerRemoval";

RequestHandlerHandle requestHandle = new RequestHandlerProcessor<>(requestName, None.class, None.class, new ElectrodeBridgeRequestHandler<None, None>() {
@Override
public void onRequest(@Nullable None payload, @NonNull ElectrodeBridgeResponseListener<None> responseListener) {
assertSame(None.NONE, payload);
responseListener.onSuccess(null);
countDownLatch.countDown();
}
}).execute();


new RequestProcessor<None, None>(requestName, null, None.class, new ElectrodeBridgeResponseListener<None>() {
@Override
public void onFailure(@NonNull FailureMessage failureMessage) {
fail();
}

@Override
public void onSuccess(@Nullable None responseData) {
assertSame(None.NONE, responseData);
countDownLatch.countDown();
}
}).execute();

requestHandle.unregister();

new RequestProcessor<None, None>(requestName, null, None.class, new ElectrodeBridgeResponseListener<None>() {
@Override
public void onFailure(@NonNull FailureMessage failureMessage) {
failureMessage.getMessage();
countDownLatch.countDown();
}

@Override
public void onSuccess(@Nullable None responseData) {
fail();
}
}).execute();

waitForCountDownToFinishOrFail(countDownLatch);
}

@Test
public void testRequestHandlerRemovalWithApi() {
final CountDownLatch countDownLatch = new CountDownLatch(3);
final int RESULT_AGE = 10;

RequestHandlerHandle handlerHandle = PersonApi.requests().registerGetAgeRequestHandler(new ElectrodeBridgeRequestHandler<String, Integer>() {
@Override
public void onRequest(@Nullable String payload, @NonNull ElectrodeBridgeResponseListener<Integer> responseListener) {
responseListener.onSuccess(RESULT_AGE);
countDownLatch.countDown();
}
});

PersonApi.requests().getAge("user", new ElectrodeBridgeResponseListener<Integer>() {
@Override
public void onSuccess(@Nullable Integer responseData) {
assertNotNull(responseData);
assertEquals(RESULT_AGE, responseData.intValue());
countDownLatch.countDown();
}

@Override
public void onFailure(@NonNull FailureMessage failureMessage) {
fail();
}
});

handlerHandle.unregister();

PersonApi.requests().getAge("user", new ElectrodeBridgeResponseListener<Integer>() {
@Override
public void onSuccess(@Nullable Integer responseData) {
fail();
}

@Override
public void onFailure(@NonNull FailureMessage failureMessage) {
assertNotNull(failureMessage);
countDownLatch.countDown();
}
});
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<String, Integer>() {
@Override
public void onRequest(@Nullable String payload, @NonNull ElectrodeBridgeResponseListener<Integer> responseListener) {
responseListener.onSuccess(RESULT_AGE_FIRST);
countDownLatch.countDown();
}
});

RequestHandlerHandle handlerHandle1 = PersonApi.requests().registerGetAgeRequestHandler(new ElectrodeBridgeRequestHandler<String, Integer>() {
@Override
public void onRequest(@Nullable String payload, @NonNull ElectrodeBridgeResponseListener<Integer> responseListener) {
responseListener.onSuccess(RESULT_AGE_SECOND);
countDownLatch.countDown();
}
});

RequestHandlerHandle handlerHandle2 = PersonApi.requests().registerGetAgeRequestHandler(new ElectrodeBridgeRequestHandler<String, Integer>() {
@Override
public void onRequest(@Nullable String payload, @NonNull ElectrodeBridgeResponseListener<Integer> responseListener) {
responseListener.onSuccess(RESULT_AGE_THIRD);
countDownLatch.countDown();
}
});

PersonApi.requests().getAge("user", new ElectrodeBridgeResponseListener<Integer>() {
@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<Integer>() {
@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<Integer>() {
@Override
public void onSuccess(@Nullable Integer responseData) {
fail();
}

@Override
public void onFailure(@NonNull FailureMessage failureMessage) {
assertNotNull(failureMessage);
countDownLatch.countDown();
}
});

waitForCountDownToFinishOrFail(countDownLatch);
}
}
91 changes: 0 additions & 91 deletions ...c/androidTest/java/com/walmartlabs/electrode/reactnative/bridge/RequestProcessorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1195,95 +1195,4 @@ public void onResponse(ReadableMap response) {

waitForCountDownToFinishOrFail(countDownLatch);
}

@Test
public void testRequestHandlerRemoval() {
final CountDownLatch countDownLatch = new CountDownLatch(3);
final String requestName = "testRequestHandlerRemoval";

RequestHandlerHandle requestHandle = new RequestHandlerProcessor<>(requestName, None.class, None.class, new ElectrodeBridgeRequestHandler<None, None>() {
@Override
public void onRequest(@Nullable None payload, @NonNull ElectrodeBridgeResponseListener<None> responseListener) {
assertSame(None.NONE, payload);
responseListener.onSuccess(null);
countDownLatch.countDown();
}
}).execute();


new RequestProcessor<None, None>(requestName, null, None.class, new ElectrodeBridgeResponseListener<None>() {
@Override
public void onFailure(@NonNull FailureMessage failureMessage) {
fail();
}

@Override
public void onSuccess(@Nullable None responseData) {
assertSame(None.NONE, responseData);
countDownLatch.countDown();
}
}).execute();

requestHandle.unregister();

new RequestProcessor<None, None>(requestName, null, None.class, new ElectrodeBridgeResponseListener<None>() {
@Override
public void onFailure(@NonNull FailureMessage failureMessage) {
failureMessage.getMessage();
countDownLatch.countDown();
}

@Override
public void onSuccess(@Nullable None responseData) {
fail();
}
}).execute();

waitForCountDownToFinishOrFail(countDownLatch);
}

@Test
public void testRequestHandlerRemovalWithApi() {
final CountDownLatch countDownLatch = new CountDownLatch(3);
final int RESULT_AGE = 10;

RequestHandlerHandle handlerHandle = PersonApi.requests().registerGetAgeRequestHandler(new ElectrodeBridgeRequestHandler<String, Integer>() {
@Override
public void onRequest(@Nullable String payload, @NonNull ElectrodeBridgeResponseListener<Integer> responseListener) {
responseListener.onSuccess(RESULT_AGE);
countDownLatch.countDown();
}
});

PersonApi.requests().getAge("user", new ElectrodeBridgeResponseListener<Integer>() {
@Override
public void onSuccess(@Nullable Integer responseData) {
assertNotNull(responseData);
assertEquals(RESULT_AGE, responseData.intValue());
countDownLatch.countDown();
}

@Override
public void onFailure(@NonNull FailureMessage failureMessage) {
fail();
}
});

handlerHandle.unregister();

PersonApi.requests().getAge("user", new ElectrodeBridgeResponseListener<Integer>() {
@Override
public void onSuccess(@Nullable Integer responseData) {
fail();
}

@Override
public void onFailure(@NonNull FailureMessage failureMessage) {
assertNotNull(failureMessage);
countDownLatch.countDown();
}
});
waitForCountDownToFinishOrFail(countDownLatch);
}

}
2 changes: 1 addition & 1 deletion ...rc/main/java/com/walmartlabs/electrode/reactnative/bridge/ElectrodeBridgeTransceiver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
3 changes: 2 additions & 1 deletion ...dge/src/main/java/com/walmartlabs/electrode/reactnative/bridge/ElectrodeNativeBridge.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package com.walmartlabs.electrode.reactnative.bridge;

import android.support.annotation.NonNull;
import android.support.annotation.Nullable;

import java.util.UUID;

Expand Down Expand Up @@ -66,7 +67,7 @@ public interface ElectrodeNativeBridge {
* @param name
* @return {@link UUID} of the request handler
*/
@NonNull
@Nullable
UUID getRequestHandlerId(@NonNull String name);

/**
Expand Down
15 changes: 5 additions & 10 deletions ...e/src/main/java/com/walmartlabs/electrode/reactnative/bridge/RequestHandlerProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this now an unexpected ("warning") case, or can it stay "debug"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops. Good catch!. typo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, I will clean up this logic now.

ElectrodeBridgeHolder.unregisterRequestHandler(id);
intermediateRequestHandler = null;
handler = null;
return true;
}
}
2 changes: 1 addition & 1 deletion ...e-bridge/src/main/java/com/walmartlabs/electrode/reactnative/bridge/RequestRegistrar.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,6 @@ public interface RequestRegistrar<T> {
* @param name
* @return {@link UUID} of the request handler
*/
@NonNull
@Nullable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, this change is breaking and would require a new major version - So question: Is this required or can you keep it NonNull? If the change is required, other parts need updates as well, e.g. the getRequestHandlerId implementation in ElectrodeBridgeTransceiver - It is still marked as NonNull, but can now return null. That in turn might require other code updates and refactoring.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was technically not used anywhere till now, So it should be okay to change the signature. I will update the ElectrodeBridgeTransceiver as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, if you look at the old implementation it was done incorrectly. A null value was returned if a match is not found.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense 👍

UUID getRequestHandlerId(@NonNull String name);
}
Loading