segmentLoader/manifestLoader automatic canRetry logic does not work

[jilles-sg]

I'm pasting links to old versions since that's what I investigated, but I have verified that the code has not changed in the dev branch.

According to the documentation at https://developers.canal-plus.com/rx-player/versions/3.32.1/doc//api/Miscellaneous/plugins.html#segmentloader and the code at https://github.com/canalplus/rx-player/blob/a457244d8777b265cf569a65264968663765d383/src/core/fetchers/utils/schedule_request.ts#L58 , there is a difference between not sending canRetry from a segmentLoader or manifestLoader and sending it with the value false: if it is absent, null or undefined, rx-player determines automatically whether to retry such as based on the HTTP response code.

However, the code at https://github.com/canalplus/rx-player/blob/a457244d8777b265cf569a65264968663765d383/src/transports/utils/call_custom_manifest_loader.ts#L105 and in other places that create CustomLoaderError changes absent, null or undefined to false. This makes the logic in schedule_request.ts unreachable, since typeof error.canRetry === "boolean" will always be true.

It would probably work to send canRetry: "automatic", but I think violating the types like that is too dangerous.

Some options are to fix it (which could be considered a small API change, although it should be benign), or to adjust the documentation and the code in schedule_request.ts to accept that the only options for canRetry are true and false, with absent, null and undefined being the same as false.

[peaBerberian]

Hi,

I think you're right, if canRetry is not set to a boolean, and if xhr is set, we could rely on the usual checking code on that xhr

I'm still not a fan of that path though as many applications usually rely on a library like axios, the fetch API or other web APIs to do the request, so asking for an xhr (only optionally but still) looks out of place here. In my opinion, when relying on a custom manifestLoader or segmentLoader, an application should write the retry conditions themselves (though I understand that they may not know CDNs and HTTP enough to know which statuses or errors would justify a retry or not, especially if they're relying on an external library for it).

But yes, xhr exist, yet canRetry: undefined do not have any different behavior than false, so there's a possible easy improvement.

To me there's no API breaking change in fixing this in the code thanks to the power of vagueness in the API documentation :p : for canRetry: undefined, we only state that it "might retry or fail depending on other factors" without specifying them, and for xhr we only say that it "can be used". This may seem far-fetched but we sometimes use such language on purpose in the API documentation to limit future breaking changes for behaviors for which we want some level of freedom (though this was here unrelated to the seen issue as we were probably more worried when writing this about the possibility of updating the actual retry conditions, not about preventing by mistake the reliance on xhr due to this bug)