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

Requesting as HEAD causes response.body to be empty #5174

Closed
wants to merge 1 commit into from
Closed

Requesting as HEAD causes response.body to be empty #5174

wants to merge 1 commit into from

Conversation

henriquebremenkanp
Copy link

Fix for: #5164

This is my first commit to shaka-player.
Please feel free to modify it if there's a better solution.

@google-cla
Copy link

google-cla bot commented Apr 25, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@avelad
Copy link
Member

avelad commented Apr 25, 2023

I don't agree with the change, @joeyparrish / @theodab , can you review it?

@erik-anderson
Copy link

The specific path I see this take is we have this getMimeType call which specifies the HEAD method. This goes through netEngine.request and it eventually reaches the shaka.net.HttpFetchPlugin's parse method. This calls shaka.net.HttpFetchPlugin.request_ which has a call to clone the response, access the body, and calls getReader().

Since body is null after the Chromium change (https://crbug.com/1297060), it throws an uncaught exception.

If this isn't the right fix, then a better one might be to update shaka.net.HttpFetchPlugin to look at init.method and, if it's HEAD, skip the path that attempts to read the body.

@theodab
Copy link
Contributor

theodab commented Apr 25, 2023

Yes, I think the alternate approach you suggested is better than this. We shouldn't be downloading the body unnecessarily, in cases where we only need the headers.

@joeyparrish
Copy link
Member

Thank you for your contribution, @henriquebremenkanp, but I agree that this is not the fix we want. It's not the same to make a full GET request when we are only probing for the MIME type. This would be wasting bandwidth, and doesn't address the root of the issue.

@henriquebremenkanp henriquebremenkanp deleted the patch-1 branch April 26, 2023 00:28
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants