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

Discard persisted events with non-nullary constructors #599

Merged
merged 3 commits into from
Sep 11, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -1385,6 +1385,33 @@ public Void apply(TestActivation.Options options) throws AblyException {
assertInstanceOf(AfterRegistrationSyncFailed.class, activation.machine.current);
}

// https://github.com/ably/ably-java/issues/598
public void test_restore_non_nullary_event() {
TestActivation activation = new TestActivation();
assertInstanceOf(NotActivated.class, activation.machine.current);

SyncRegistrationFailed event = new SyncRegistrationFailed(new ErrorInfo());

activation.machine.handleEvent(event);

// NotActivated can't handle SyncRegistrationFailed, so it should be pending.
assertTrue(activation.machine.pendingEvents.contains(event));

// Now recover the persisted state and events.

activation = new TestActivation(new Helpers.AblyFunction<TestActivation.Options, Void>() {
@Override
public Void apply(TestActivation.Options options) throws AblyException {
options.clearPersisted = false;
return null;
}
});

// Since the event doesn't have a nullary constructor, it should be dropped.
assertInstanceOf(NotActivated.class, activation.machine.current);
assertSize(0, activation.machine.pendingEvents);
}

// This is all copied and pasted from ParameterizedTest, since I can't inherit from it.
// I need to inherit from AndroidPushTest, and Java doesn't have multiple inheritance
// or mixins or something like that.
Expand Down
18 changes: 16 additions & 2 deletions android/src/main/java/io/ably/lib/push/ActivationStateMachine.java
Original file line number Diff line number Diff line change
Expand Up @@ -573,6 +573,9 @@ public synchronized boolean handleEvent(ActivationStateMachine.Event event) {
// An event's side effects may end up synchronously calling handleEvent while it's
// itself being handled. In that case, enqueue it so it's handled next (and still
// synchronously).
//
// We don't need to persist here, as the handleEvent call up the stack will eventually
// persist when done with the synchronous transitions.
enqueueEvent(event);
return true;
}
Expand All @@ -584,7 +587,7 @@ public synchronized boolean handleEvent(ActivationStateMachine.Event event) {
ActivationStateMachine.State maybeNext = current.transition(event);
if (maybeNext == null) {
enqueueEvent(event);
return true;
return persist();
}

Log.d(TAG, String.format("transition: %s -(%s)-> %s", current.getClass().getSimpleName(), event.getClass().getSimpleName(), maybeNext.getClass().getSimpleName()));
Expand Down Expand Up @@ -675,7 +678,18 @@ private ArrayDeque<ActivationStateMachine.Event> getPersistedPendingEvents() {
for (int i = 0; i < length; i++) {
try {
String className = activationContext.getPreferences().getString(String.format("%s[%d]", ActivationStateMachine.PersistKeys.PENDING_EVENTS_PREFIX, i), "");
ActivationStateMachine.Event event = ((Class<ActivationStateMachine.Event>) Class.forName(className)).newInstance();
Class<ActivationStateMachine.Event> eventClass = (Class<ActivationStateMachine.Event>) Class.forName(className);
ActivationStateMachine.Event event;
try {
event = eventClass.newInstance();
Copy link
Member

Choose a reason for hiding this comment

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

You wouldn't prefer an isPersisted() or something on the event?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@paddybyers All of them are about to become persistent when #546 is fixed, so we'd then have to find another way to detect "old" persisted stated.

} catch(InstantiationException e) {
// We aren't properly persisting events with a non-nullary constructor. Those events
// are supposed to be handled by states that aren't persisted (until
// https://github.com/ably/ably-java/issues/546 is fixed), so it should be safe to
// just drop them.
Log.d(TAG, String.format("discarding improperly persisted event: %s", className));
continue;
}
deque.add(event);
} catch(Exception e) {
throw new RuntimeException(e);
Expand Down