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

Allow async modifications to requests. #3957

Closed
ElasticPencil opened this issue Jun 8, 2022 · 5 comments
Closed

Allow async modifications to requests. #3957

ElasticPencil opened this issue Jun 8, 2022 · 5 comments

Comments

@ElasticPencil
Copy link

Is your feature request related to a problem? Please describe.
When requesting videos from the service we need to provide an authorization header. This is a standard Bearer token and it has a reasonably short lifetime (1 hour). When we begin playing we may have a valid token but it can expire while playing and requesting new buffers requires a new token. This means we need to make an initial request to refresh the token.

Describe the solution you'd like
Allowing request modifiers to return a promise, or passing in a callback that can be used to signal the modification is complete would allow this type of behavior.

Describe alternatives you've considered
The only alternative I know would be to generate a new token just before playing and pre-generate new ones before the time comes up.

@dsilhavy
Copy link
Collaborator

Duplicate of #3758.

@ElasticPencil Is this something you can work on and provide as a pull request?

@DakotaEmber
Copy link

I will look into when I get time to focus on our improved video playback.

@littlespex
Copy link
Contributor

littlespex commented Aug 11, 2022

Here is a PR with a potential solution, which I've tested locally and verified it works. (Hide the whitespace to see the changes more clearly):

https://github.com/Dash-Industry-Forum/dash.js/pull/4024/files?diff=unified&w=1

@dsilhavy To avoid trashing too many files, this solution adds a new optional method to the RequestModifier class called modifyRequest. The function is a transform/map accepting a Request object (limited to url, headers, credentials). If the function returns an object, the loader will apply the values to the httpRequest. There are a few open questions:

  • I'm not sure an optional function fits with the rest of the architecture. Maybe add a separate config option?
  • Should the modifyRequest function mutate the given request object, or return a it's own request?
  • How does this effect timing stats? The modifier is applied before requestStartTime is recorded, so the download stats should function as they did before, but will any extra delay in the async function mess with any other calculations? If so, does it matter, or can we add a note in the docs like: "If you use async modifiers, stats may be adversely effected".

I'm open to feedback.

@ElasticPencil
Copy link
Author

I left some comments in the PR. I like the pattern you implemented. With regards to the stats, I don't have a strong opinion. I think it is a little odd that the time is started before synch modifiers but doesn't include async modifiers. That said I don't think it will affect the system overall. It may be better to leave it the way it is. One thing that could be done, is move the timing before, but don't call the utility modifyRequest unless there is a handler setup. This would mean the time effectively wouldn't change for existing code. It would only be impacted when a modifyRequest handler was added.

@dsilhavy
Copy link
Collaborator

Implemented, sample available here: https://reference.dashif.org/dash.js/nightly/samples/advanced/extend.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants