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

There is fixed the issue of reconnection. autoConnectMs was changed… #25

Merged
merged 1 commit into from
Jul 18, 2022

Conversation

vnabiev
Copy link
Collaborator

@vnabiev vnabiev commented Jul 15, 2022

… by ReconnectionPolicy.

WsProvider couldn't reconnect to the web socket if it was closed - future whenConnected wasn't being completed when socket closes that blocked reconnection.

@vnabiev vnabiev requested a review from kalaninja July 15, 2022 16:34

@Override
public void reset() {

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// nothing to reset

@@ -575,17 +585,24 @@ public Builder setResponseTimeout(long timeout, TimeUnit timeUnit) {
return this;
}

public Builder withPolicy(ReconnectionPolicy policy) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since there might be different policies for different reasons, the name might become confusing. What do you think about reconnectWith()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't see any problem. I think in that case it might be achieved by using overloads. Your proposal is also ok for me.

@RequiredArgsConstructor(access = AccessLevel.PRIVATE)
@Getter
@Slf4j
public class BackoffReconnectionPolicy implements ReconnectionPolicy {
Copy link
Contributor

Choose a reason for hiding this comment

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

Backoff doesn't tell me anything about the algorithm being used. How about ExponentialReconnectionPolicy?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The policy allows to use a constant time backoff also that's why I called it like this. I think "exponential backoff" is more common name so will rename to ExponentialBackoffPolicy or ExponentialBackoffReconnectionPolicy.

return ReconnectionSchedule.NEVER;
}

var nextDelay = delay * Math.pow(factor, counter);
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I can see negatives and numbers less than one are allowed, which might result in a tricky behavior. Is it intentional?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Negatives are not allowed there. I left an opportunity to use numbers less than one intentionally, but probably it doesn't make much sense.

* @param context contains a reason of disconnection
* @return a schedule planned for reconnection
*/
@NonNull ReconnectionSchedule apply(@NonNull ReconnectionContext context);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the context being used anywhere. Also probably the policy should be affected by the reason of disconnection.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, I didn't get your point.

/**
* The method is called when connection successfully open
*/
void reset();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this method implies that the policy is stateful. I can suggest doing it a little different. The policy holds an algorithm, which generates a schedule providing you with next timeslots. If you need to reset you just generate a new schedule.

public interface ReconnectionPolicy {
    interface ReconnectionSchedule {
        long getNextDelay();
        TimeUnit getTimeUnit();
    }

    ReconnectionSchedule getSchedule();
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, it probably might be better if a consumer can tell the schedule which time units it wants to see the results in instead of being made to use time units predefined by the author of the policy.
long getNextDelay(TimeUnit timeUnit)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for these suggestions!
With regard to the proposal to use long getNextDelay(TimeUnit timeUnit), in general it won't work. E.g., let's assume somebody sets delay as "30 seconds", but a consumer of the policy requests in minutes so they would get "0 minutes" instead of the correct value. I would use the class Duration, however it's not convenient for scheduling. I agree that to have a stateless policy is more preferable, but the suggested solution is not so flexible, e.g. for decorating the policy. I decided to change the signature a little. A policy initializes and provides a required context (the method is requested before the first connection or when connection reestablished), which will be passed to the one after every failure.

@vnabiev vnabiev force-pushed the fix/reconnection branch from 7916dfb to 103f822 Compare July 18, 2022 20:57
… by `ReconnectionPolicy`.

WsProvider couldn't reconnect to the web socket if it was closed - future `whenConnected` wasn't being completed when socket closes that blocked reconnection.
@vnabiev vnabiev force-pushed the fix/reconnection branch from 103f822 to 2713c4f Compare July 18, 2022 21:14
@vnabiev vnabiev merged commit 01002ce into develop Jul 18, 2022
@vnabiev vnabiev deleted the fix/reconnection branch July 18, 2022 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants