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

Jump to bottom

[netatmo] API limit reached handling #16489

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
+26 −23
Open

[netatmo] API limit reached handling #16489

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
86a0837
Enhance API limit reached handling
clinique Mar 5, 2024
6048eef
Debugging leftower removed
clinique Mar 5, 2024
26d3c74
Improve handling of interrupted request by alllowing retries
clinique Mar 5, 2024
4d87b7f
Working on InterruptedException
clinique Mar 6, 2024
3242aa7
Improve handling of interrupted request by alllowing retries
clinique Mar 25, 2024
4870e57
Improve handling of interrupted request by alllowing retries
clinique Mar 25, 2024
a189039
Improve handling of interrupted request by alllowing retries
clinique Mar 29, 2024
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
45 changes: 23 additions & 22 deletions ....netatmo/src/main/java/org/openhab/binding/netatmo/internal/handler/ApiBridgeHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@
@NonNullByDefault
public class ApiBridgeHandler extends BaseBridgeHandler {
private static final int TIMEOUT_S = 20;
private static final int API_LIMIT_INTERVAL_S = 3600;

private final Logger logger = LoggerFactory.getLogger(ApiBridgeHandler.class);
private final AuthenticationApi connectApi = new AuthenticationApi(this);
Expand Down Expand Up @@ -210,8 +211,7 @@ private boolean authenticate(@Nullable String code, @Nullable String redirectUri
startAuthorizationFlow();
return false;
} catch (IOException e) {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, e.getMessage());
prepareReconnection(code, redirectUri);
prepareReconnection(getConfiguration().reconnectInterval, e.getMessage(), code, redirectUri);
return false;
}

Expand Down Expand Up @@ -239,11 +239,13 @@ public ApiHandlerConfiguration getConfiguration() {
return getConfigAs(ApiHandlerConfiguration.class);
}

private void prepareReconnection(@Nullable String code, @Nullable String redirectUri) {
private void prepareReconnection(int delay, @Nullable String message, @Nullable String code,
@Nullable String redirectUri) {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, message);
Copy link
Contributor

Choose a reason for hiding this comment

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

Status is now set twice when authentication fails - see #16489 (comment)

It's set by startAuthorizationFlow, and then again here when called from line 312.

connectApi.dispose();
freeConnectJob();
connectJob = Optional.of(scheduler.schedule(() -> openConnection(code, redirectUri),
getConfiguration().reconnectInterval, TimeUnit.SECONDS));
connectJob = Optional.of(scheduler.schedule(() -> openConnection(code, redirectUri), delay, TimeUnit.SECONDS));
logger.debug("Reconnection scheduled in {} seconds", delay);
}

private void freeConnectJob() {
Expand Down Expand Up @@ -307,7 +309,7 @@ public synchronized <T> T executeUri(URI uri, HttpMethod method, Class<T> clazz,
Request request = httpClient.newRequest(uri).method(method).timeout(TIMEOUT_S, TimeUnit.SECONDS);

if (!authenticate(null, null)) {
prepareReconnection(null, null);
prepareReconnection(getConfiguration().reconnectInterval, null, null, "@text/status-bridge-offline");
throw new NetatmoException("Not authenticated");
}
connectApi.getAuthorization().ifPresent(auth -> request.header(HttpHeader.AUTHORIZATION, auth));
Expand Down Expand Up @@ -348,27 +350,26 @@ public synchronized <T> T executeUri(URI uri, HttpMethod method, Class<T> clazz,
try {
exception = new NetatmoException(deserializer.deserialize(ApiError.class, responseBody));
} catch (NetatmoException e) {
exception = new NetatmoException("Error deserializing error: %s".formatted(statusCode.getMessage()));
if (statusCode == Code.TOO_MANY_REQUESTS) {
exception = new NetatmoException(statusCode.getMessage());
} else {
exception = new NetatmoException(
"Error deserializing error: %s".formatted(statusCode.getMessage()));
}
}
throw exception;
} catch (NetatmoException e) {
if (e.getStatusCode() == ServiceError.MAXIMUM_USAGE_REACHED) {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, e.getMessage());
prepareReconnection(null, null);
if (statusCode == Code.TOO_MANY_REQUESTS
|| exception.getStatusCode() == ServiceError.MAXIMUM_USAGE_REACHED) {
prepareReconnection(API_LIMIT_INTERVAL_S,
clinique marked this conversation as resolved.
Show resolved Hide resolved
"@text/maximum-usage-reached [ \"%d\" ]".formatted(API_LIMIT_INTERVAL_S), null, null);
}
throw e;
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, e.getMessage());
throw new NetatmoException("Request interrupted");
} catch (TimeoutException | ExecutionException e) {
throw exception;
} catch (InterruptedException | TimeoutException | ExecutionException e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure the InterruptedException handling is correct, but I think it was not correct previously either because the handler probably didn't respect it after being converted to NetatmoException. This might give a bit insight: https://rules.sonarsource.com/java/RSPEC-2142/

By initiating a retry below, it seems we are delaying the thread interruption. Perhaps it would be better to propagate the exception?

@wborn - can you advise a bit here?

if (retryCount > 0) {
logger.debug("Request timedout, retry counter: {}", retryCount);
logger.debug("Request error, retry counter: {}", retryCount);
return executeUri(uri, method, clazz, payload, contentType, retryCount - 1);
}
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, "@text/request-time-out");
prepareReconnection(null, null);
throw new NetatmoException(String.format("%s: \"%s\"", e.getClass().getName(), e.getMessage()));
prepareReconnection(getConfiguration().reconnectInterval, "@text/request-time-out", null, e.getMessage());
throw new NetatmoException("%s: \"%s\"".formatted(e.getClass().getName(), e.getMessage()));
}
}

Expand Down
1 change: 1 addition & 0 deletions ...in/java/org/openhab/binding/netatmo/internal/handler/capability/AlarmEventCapability.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ protected void updateWebhookEvent(WebhookEvent event) {
handler.updateState(GROUP_LAST_EVENT, CHANNEL_EVENT_TIME, toDateTimeType(event.getTime()));
handler.updateState(GROUP_LAST_EVENT, CHANNEL_EVENT_SUBTYPE,
event.getSubTypeDescription().map(ChannelTypeUtils::toStringType).orElse(UnDefType.NULL));

final String message = event.getName();
handler.updateState(GROUP_LAST_EVENT, CHANNEL_EVENT_MESSAGE,
message == null || message.isBlank() ? UnDefType.NULL : toStringType(message));
Expand Down
2 changes: 1 addition & 1 deletion ...c/main/java/org/openhab/binding/netatmo/internal/handler/capability/CameraCapability.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ public CameraCapability(CommonInterface handler, NetatmoDescriptionProvider desc
List<ChannelHelper> channelHelpers) {
super(handler, descriptionProvider, channelHelpers);
this.personChannelUID = new ChannelUID(thingUID, GROUP_LAST_EVENT, CHANNEL_EVENT_PERSON_ID);
this.cameraHelper = (CameraChannelHelper) channelHelpers.stream().filter(c -> c instanceof CameraChannelHelper)
this.cameraHelper = (CameraChannelHelper) channelHelpers.stream().filter(CameraChannelHelper.class::isInstance)
.findFirst().orElseThrow(() -> new IllegalArgumentException(
"CameraCapability must find a CameraChannelHelper, please file a bug report."));
}
Expand Down
1 change: 1 addition & 0 deletions bundles/org.openhab.binding.netatmo/src/main/resources/OH-INF/i18n/netatmo.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -465,6 +465,7 @@ device-not-connected = Thing is not reachable
data-over-limit = Data seems quite old
request-time-out = Request timed out - will attempt reconnection later
deserialization-unknown = Deserialization lead to an unknown code
maximum-usage-reached = Maximum usage reached, will reconnect in {0} seconds.

homestatus-unknown-error = Unknown error
homestatus-internal-error = Internal error
Expand Down