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

Modified PNReconnectionPolicy #297

Merged
merged 6 commits into from
Dec 18, 2023
Merged

Modified PNReconnectionPolicy #297

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4e5f052
Modified PNReconnectionPolicy
marcin-cebo Dec 15, 2023
3bb2e89
Changes after review.
marcin-cebo Dec 18, 2023
ea92293
Changes after review.
marcin-cebo Dec 18, 2023
93ffeba
Changes after review.
marcin-cebo Dec 18, 2023
900b682
Merge branch 'master' into mc_retryPolic_tataSky
marcin-cebo Dec 18, 2023
09a817a
PubNub SDK v6.4.5 release.
pubnub-release-bot Dec 18, 2023
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
7 changes: 5 additions & 2 deletions src/main/java/com/pubnub/api/PNConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ public class PNConfiguration {
private static final int SUBSCRIBE_TIMEOUT = 310;
private static final int CONNECT_TIMEOUT = 5;
private static final int FILE_MESSAGE_PUBLISH_RETRY_LIMIT = 5;
private static final int MAXIMUM_RECONNECTION_RETRIES_DEFAULT = -1; // infinite

@Getter
private SSLSocketFactory sslSocketFactory;
Expand Down Expand Up @@ -201,7 +202,9 @@ public void setUserId(@NotNull UserId userId) {
private PNReconnectionPolicy reconnectionPolicy;

/**
* Set how many times the reconneciton manager will try to connect before giving app
* Set how many times the reconnection manager will try to connect before giving up.
* Max allowed value is 10.
* Default is 10.
wkal-pubnub marked this conversation as resolved.
Show resolved Hide resolved
wkal-pubnub marked this conversation as resolved.
Show resolved Hide resolved
*/
@Setter
private int maximumReconnectionRetries;
Expand Down Expand Up @@ -284,7 +287,7 @@ public PNConfiguration(@NotNull UserId userId) throws PubNubException {

startSubscriberThread = true;

maximumReconnectionRetries = -1;
maximumReconnectionRetries = MAXIMUM_RECONNECTION_RETRIES_DEFAULT;

dedupOnSubscribe = false;
suppressLeaveEvents = false;
Expand Down
13 changes: 10 additions & 3 deletions src/main/java/com/pubnub/api/managers/DelayedReconnectionManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,19 @@
import com.pubnub.api.enums.PNReconnectionPolicy;
import lombok.extern.slf4j.Slf4j;

import java.util.Random;
import java.util.Timer;
import java.util.TimerTask;

@Slf4j
public class DelayedReconnectionManager {
private static final int DELAY_SECONDS = 3;
private static final int MILLISECONDS = 1000;
private static final int BASE_DELAY_MILLISECONDS = 2000;
private static final int BOUND = 1000;

private final PNReconnectionPolicy pnReconnectionPolicy;
private ReconnectionCallback callback;
private PubNub pubnub;
private final Random random = new Random();

/**
* Timer for heartbeat operations.
Expand All @@ -34,12 +36,13 @@ public void scheduleDelayedReconnection() {
}

timer = new Timer("Delayed Reconnection Manager timer", true);
int effectiveDelayInMilliSeconds = (int) (BASE_DELAY_MILLISECONDS + getRandomDelayInMilliSeconds());
timer.schedule(new TimerTask() {
@Override
public void run() {
callTime();
}
}, DELAY_SECONDS * MILLISECONDS);
}, effectiveDelayInMilliSeconds);
}

public void setReconnectionListener(ReconnectionCallback reconnectionCallback) {
Expand All @@ -61,6 +64,10 @@ private boolean isReconnectionPolicyUndefined() {
return false;
}

private double getRandomDelayInMilliSeconds() {
wkal-pubnub marked this conversation as resolved.
Show resolved Hide resolved
return random.nextInt(BOUND);
}

private void callTime() {
stop();
callback.onReconnection();
Expand Down
40 changes: 26 additions & 14 deletions src/main/java/com/pubnub/api/managers/ReconnectionManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,21 @@
import org.jetbrains.annotations.NotNull;

import java.util.Calendar;
import java.util.Random;
import java.util.Timer;
import java.util.TimerTask;


@Slf4j
public class ReconnectionManager {

private static final int LINEAR_INTERVAL = 3;
private static final int MIN_EXPONENTIAL_BACKOFF = 1;
private static final int BASE_LINEAR_INTERVAL_IN_MILLISECONDS = 3000;
private static final int MIN_EXPONENTIAL_BACKOFF = 2;
private static final int MAX_EXPONENTIAL_BACKOFF = 32;

private static final int MILLISECONDS = 1000;
private static final int BOUND = 1000;
private static final int MILLISECONDS = BOUND;
private static final int MAXIMUM_RECONNECTION_RETRIES_DEFAULT = 10;

private ReconnectionCallback callback;
private PubNub pubnub;
Expand All @@ -31,6 +34,7 @@ public class ReconnectionManager {

private PNReconnectionPolicy pnReconnectionPolicy;
private int maxConnectionRetries;
private final Random random = new Random();

/**
* Timer for heartbeat operations.
Expand All @@ -55,18 +59,18 @@ public void startPolling() {
exponentialMultiplier = 1;
failedCalls = 0;

registerHeartbeatTimer();
registerRetryTimer();
}

private void registerHeartbeatTimer() {
private void registerRetryTimer() {
// make sure only one timer is running at a time.
stopHeartbeatTimer();

if (isReconnectionPolicyUndefined()) {
return;
}

if (maxConnectionRetries != -1 && failedCalls >= maxConnectionRetries) { // _what's -1?
if (maxConnectionIsNotSetToInfinite() && failedCalls >= maxConnectionRetries) {
callback.onMaxReconnectionExhaustion();
return;
}
Expand All @@ -78,11 +82,15 @@ private void registerHeartbeatTimer() {
public void run() {
callTime();
}
}, getNextInterval() * MILLISECONDS);
}, getNextIntervalInMilliSeconds());
}

int getNextInterval() {
int timerInterval = LINEAR_INTERVAL;
private boolean maxConnectionIsNotSetToInfinite() {
Copy link
Contributor

Choose a reason for hiding this comment

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

having negation (Not) in the middle of the name here is confusing,
how about isInfiniteReconnections ? (and flip the if to ==, and fix call sites)

Copy link
Contributor Author

@marcin-cebo marcin-cebo Dec 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

But then condition would change to:
if (!maxConnectionIsSetToInfinite() && failedCalls >= maxConnectionRetries) {

For me it is more clear to have
if (maxConnectionIsNotSetToInfinite() && failedCalls >= maxConnectionRetries) {
than
if (!maxConnectionIsSetToInfinite() && failedCalls >= maxConnectionRetries) {

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would always try to use affirmative names for boolean values (names and functions) where possible
!isSomething or even !(isSomething) for clarity is perfectly fine

with negative names there is a possibility to get double negatives which is really confusing, such as !isNotSomething()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For me maxConnectionIsNotSetToInfinite reads better. I created this function to describe this mysterious condition:
maxConnectionRetries != -1
I assumed the function will not be reused. In situation !isNotSomething() is needed I would refactor it.

Anyway I will change it to maxConnectionIsSetToInfinite

return maxConnectionRetries != -1;
}

int getNextIntervalInMilliSeconds() {
int timerInterval = 0;
failedCalls++;

if (pnReconnectionPolicy == PNReconnectionPolicy.EXPONENTIAL) {
Expand All @@ -91,20 +99,24 @@ int getNextInterval() {
if (timerInterval > MAX_EXPONENTIAL_BACKOFF) {
timerInterval = MIN_EXPONENTIAL_BACKOFF;
exponentialMultiplier = 1;
log.debug("timerInterval > MAXEXPONENTIALBACKOFF at: " + Calendar.getInstance().getTime().toString());
log.debug("timerInterval > MAXEXPONENTIALBACKOFF at: " + Calendar.getInstance().getTime());
} else if (timerInterval < 1) {
timerInterval = MIN_EXPONENTIAL_BACKOFF;
}
log.debug("timerInterval = " + timerInterval + " at: " + Calendar.getInstance().getTime().toString());
timerInterval = (int) ((timerInterval * MILLISECONDS) + getRandomDelayInMilliSeconds());
log.debug("timerInterval = " + timerInterval + "ms at: " + Calendar.getInstance().getTime());
}

if (pnReconnectionPolicy == PNReconnectionPolicy.LINEAR) {
timerInterval = LINEAR_INTERVAL;
timerInterval = (int) (BASE_LINEAR_INTERVAL_IN_MILLISECONDS + getRandomDelayInMilliSeconds());
}

return timerInterval;
}

private double getRandomDelayInMilliSeconds() {
wkal-pubnub marked this conversation as resolved.
Show resolved Hide resolved
return random.nextInt(BOUND);
}

private void stopHeartbeatTimer() {
if (timer != null) {
timer.cancel();
Expand All @@ -121,7 +133,7 @@ public void onResponse(PNTimeResult result, @NotNull PNStatus status) {
callback.onReconnection();
} else {
log.debug("callTime() at: " + Calendar.getInstance().getTime().toString());
registerHeartbeatTimer();
registerRetryTimer();
}
}
});
Expand Down
14 changes: 7 additions & 7 deletions src/test/java/com/pubnub/api/managers/ReconnectionManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@

import java.util.UUID;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;

public class ReconnectionManagerTest {
Expand All @@ -20,10 +19,11 @@ public void reconnectionIntervalsEqualsForLinear() throws PubNubException {
pnConfiguration.setReconnectionPolicy(PNReconnectionPolicy.LINEAR);
final ReconnectionManager reconnectionManagerUnderTest = new ReconnectionManager(pubNub);

int firstInterval = reconnectionManagerUnderTest.getNextInterval();
int secondInterval = reconnectionManagerUnderTest.getNextInterval();
int firstInterval = reconnectionManagerUnderTest.getNextIntervalInMilliSeconds();
int secondInterval = reconnectionManagerUnderTest.getNextIntervalInMilliSeconds();

assertEquals(secondInterval, firstInterval);
assertTrue(firstInterval >= 3000 && firstInterval <= 6000);
assertTrue(secondInterval >= 3000 && secondInterval <= 6000);
}

@Test
Expand All @@ -33,9 +33,9 @@ public void reconnectionIntervalsIncreaseForExponential() throws PubNubException
PubNub pubNub = new PubNub(pnConfiguration);
final ReconnectionManager reconnectionManagerUnderTest = new ReconnectionManager(pubNub);

int firstInterval = reconnectionManagerUnderTest.getNextInterval();
int secondInterval = reconnectionManagerUnderTest.getNextInterval();
int thirdInterval = reconnectionManagerUnderTest.getNextInterval();
int firstInterval = reconnectionManagerUnderTest.getNextIntervalInMilliSeconds();
int secondInterval = reconnectionManagerUnderTest.getNextIntervalInMilliSeconds();
int thirdInterval = reconnectionManagerUnderTest.getNextIntervalInMilliSeconds();

assertTrue(firstInterval < secondInterval);
assertTrue(secondInterval < thirdInterval);
Expand Down
Loading