Add retry logic for ECP requests, on failure return our own error to make it more clear what failed #123
Conversation
…make it more clear what failed
| public async getActiveApp() { | ||
| const result = await this.device.sendEcpGet(`query/active-app`); | ||
| public async getActiveApp(options: HttpRequestOptions = {}) { | ||
| const result = await this.device.sendEcpGet(`query/active-app`, undefined, options); |
There was a problem hiding this comment.
I wish we didn't have to pass undefined for param3. :(
There was a problem hiding this comment.
We could switch the second param to have something be something like
{
params: params
options: options
}
but that would be a breaking change for anyone using sendEcpGet
There was a problem hiding this comment.
Yeah. I had discussed how to fix that in another comment, but you answered that as well. All good.
| @@ -102,26 +107,40 @@ export class RokuDevice { | |||
| return updatedContents; | |||
| } | |||
|
|
|||
| public sendEcpPost(path: string, params = {}, body: needle.BodyData = ''): Promise<needle.NeedleResponse> { | |||
| return this.sendEcp(path, params, body); | |||
| public sendEcpPost(path: string, params = {}, body: needle.BodyData = '', options: HttpRequestOptions = {}): Promise<needle.NeedleResponse> { | |||
There was a problem hiding this comment.
Could we change the third parameter to be a single options object instead of needing to add a 4th param? I see you set needle.bodyData = ''. Will it always be a string? If so, we could maintain backwards compatibility by doing something like this:
public sendEcpPost(path: string, params = {}, options: SomethingHere) {
let body: needle.BodyData;
let actualOptions: HttpRequestOptions;
if (typeof options === '') {
body = options;
} else {
body = options?.body ?? '';
actualOptions = options;
}There was a problem hiding this comment.
No body can be any of Buffer | KeyValue | NodeJS.ReadableStream | string | null; Often times it is passed as an object and it will automatically add the json header and encode it for you.
There was a problem hiding this comment.
okay. well then, I guess it's probably gotta stay the way you implemented it.
This is been one of the issues I've struggled to track down for a while as it doesn't happen often and when it does the only message was
socket hang up. I thought this was due to our socket logic between our code on the client and the component running on the device but this proved not to be true. We were not handling any errors when an ECP request fails and this was causing it ultimately bubble up thesocket hang uperror inside of Node. Now we will attempt to retry the request if retryCount is more than 0 and will throw a more useful error for the person running into the issue.