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

docs: hide Range header from flask to workaround beets web bug #41

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mgoltzsche
Copy link
Contributor

@mgoltzsche mgoltzsche commented Dec 9, 2023

This is to prevent flask from handling range requests when nginx emulates range requests.
Playing large opus files within firefox and gstreamer/mopidy worked for me only after I made this change in my nginx config.
I guess this was not a problem previously since older flask versions did not support range queries at all but that seems to have changed partially since flask returns an Accept-Ranges: bytes header.
Though, when using flask for streaming directly (without reverse-proxy), audio still doesn't play within firefox or gstreamer.
gstreamer logged an error indicating the request is unsupported.

@mgoltzsche mgoltzsche force-pushed the enhance-nginx-conf branch 2 times, most recently from 6811070 to 3a661e4 Compare December 9, 2023 01:39
@mgoltzsche mgoltzsche changed the title docs: enhance nginx.conf to work with flask 3 docs: make nginx.conf work with flask 3 Dec 9, 2023
@mgoltzsche mgoltzsche changed the title docs: make nginx.conf work with flask 3 docs: prevent flask from handling range requests Dec 11, 2023
@mgoltzsche
Copy link
Contributor Author

mgoltzsche commented Dec 11, 2023

I just corrected my earlier assumption that streaming wasn't working because of flask 3 usage: Turns out I didn't use flask 3 actually but still 2. Nevertheless, streaming works with gstreamer and firefix only after I made this change.

@mgoltzsche mgoltzsche changed the title docs: prevent flask from handling range requests docs: support range requests w. gstreamer/firefox Dec 11, 2023
@mgoltzsche mgoltzsche changed the title docs: support range requests w. gstreamer/firefox docs: support range requests with gstreamer/firefox Dec 11, 2023
@sumpfralle
Copy link
Contributor

Thank you for investigating this issue!

Your changes hide all range-related headers from flask : this sounds like a necessary addition to the previous directive (proxy_force_ranges on;). Good!

I feel a bit uncomfortable with adding the comment flask does not fully support range requests without a specific issue reference (in flask). Right now I did not find a range-related issue there.
Maybe you could open an issue with a request example (either here or in the flask issue tracker) and reference this issue in your comment?
That would be neat!

(otherwise I would just merge your proposal as it is, since it is an obvious improvement, anyway)

@mgoltzsche mgoltzsche force-pushed the enhance-nginx-conf branch 3 times, most recently from e528018 to 414123f Compare December 12, 2023 22:36
@mgoltzsche mgoltzsche changed the title docs: support range requests with gstreamer/firefox docs: hide range-related headers from flask Dec 12, 2023
@mgoltzsche
Copy link
Contributor Author

I removed my unconfirmed suspicion from the comment now. Maybe flask supports range requests now but there was another problem in my setup/config? I won't investigate that now since I need an nginx reverse-proxy in front of it anyway now but I may come back to that later at some point eventually.

@kingosticks
Copy link
Member

kingosticks commented Dec 13, 2023

Flask does support range requests and partial responses, but Beets web plugin does not. The plugin ignores any range request and will send the entire file each time (see here). Without any sort of proxy sorting this out (e.g. proxy_force_ranges on), GStreamer will be expecting a 206 Partial Content response code to its range request but be getting a regular 200 response, which indicates the request was not supported.

Given the plugin does not support partial responses, it should not advertise that it does. So it should not be responding with "Accept-Ranges: bytes". However, I can't see where it sets that response header. Do you have a log showing the traffic between GStreamer and Beets?

It's interesting you need the extra nginx config settings, I would have thought proxy_force_ranges on is enough to work around this. And also it's weird that beets cares about the headers you've removed and added. I don't understand that.

@mgoltzsche
Copy link
Contributor Author

mgoltzsche commented Dec 19, 2023

The link you posted shows that

Thus, looking at the code, it seems flask fully supports range requests indeed but the beets web plugin breaks them by overwriting the Content-Length header after it was set by werkzeug, always setting the full file length instead of sending the length of the requested range/chunk that werkzeug has set.
Hence, the proper solution might be to remove the line that overwrites the Content-Length header within the beets web plugin. I'll try that...

@kingosticks
Copy link
Member

kingosticks commented Dec 20, 2023

I still can't see where that header processing function is called. But it would make sense that it is called. There was some flask issue showing a wrapped version of sendfile that handled range requests, it was more involved than just not overwriting that header but maybe it was old code. I don't use any of this, i was just curious!

@mgoltzsche
Copy link
Contributor Author

mgoltzsche commented Dec 20, 2023

Removing that line within the beets web plugin really fixed the problem. I created a corresponding beets PR now.

I suggest to merge this PR (since it is a valid workaround currently) and create/merge another one removing the workaround section within the readme as soon as the beet web plugin fix has been released (I anticipate it will be shipped with the beets 1.6.1 release).

@mgoltzsche
Copy link
Contributor Author

mgoltzsche commented Dec 20, 2023

fyi the header processing is part of the response_class which is constructed here. A few lines below, the make_conditional method is called with accept_ranges=True when conditional is True which is the default. That method then calls _process_range_request here which implements the range request handling and setting of those headers.

@mgoltzsche mgoltzsche changed the title docs: hide range-related headers from flask docs: hide Range header from flask to workaround beets web bug Dec 26, 2023
@mgoltzsche
Copy link
Contributor Author

mgoltzsche commented Dec 26, 2023

I just updated the PR, simplifying the workaround, explaining that it is required prior to beets 1.6.1 only and linking to the PR that fixes the beets web plugin bug. Please re-review!

This is to prevent flask from handling range requests when nginx is emulates range responses.
Playing large opus files within firefox and gstreamer/mopidy worked for me only after I made this change in my nginx config.
When using flask for streaming directly (without reverse-proxy), audio still doesn't play within firefox or gstreamer - prior to beets 1.6.1.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants