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

Increase snapshot handler warning timeout to 8000ms. #1055

Merged
merged 1 commit into from
Jul 6, 2024
Merged

Increase snapshot handler warning timeout to 8000ms. #1055

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion src/lib/controller/CameraController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1025,7 +1025,7 @@ export class CameraController extends EventEmitter implements SerializableContro
reject(HAPStatus.OPERATION_TIMED_OUT);
}, 17000);
timeout.unref();
}, 5000);
}, 8000);
Copy link
Member

Choose a reason for hiding this comment

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

This timeout is for the „slow to respond“ message. The problem with snapshot handlers is that they will block the accessory from receiving further request from the device.
Also currently, we do a 5000+17000 max timeout (after which we return a HAPStatue Timeout). If we really are increasing the timeout to 8s we might want to decrease the 17s one?

Copy link
Contributor Author

@hjdhjd hjdhjd Jul 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I opted to leave the one at 17s because it seems reasonable. I understand that they may block the accessory from receiving further requests - that’s the plugin developer’s problem, not Homebridge’s. I don’t want Homebridge telling me what I already know. 😄 In my use case, I know that they may take longer than 5 seconds to complete - that doesn’t make it wrong or invalid and it leads to unnecessary end-user questions. There’s currently no way to turn these off or configure them to more properly be tuned to an individual plugin’s needs. That’s for another day and then the plugin developer can decide how much alerting (or not) they want. That’s the right long-term answer, but in the short-term, I think this is reasonable.

TL;DR: I think 17s is good as it is, with a warning at the ~45% mark (8s) and a timeout at 17s (100% for the purposes of this conversation). A warning at the ~17% mark (5s) is overly aggressive. It’s a reasonable compromise.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good, thanks for explaining the rationale behind this change 👍

timeout.unref();

try {
Expand Down
Loading