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

Fix #244: Disable buffer for comments #247

Conversation

dellagustin
Copy link
Contributor

Comments for an episode are sent back to the front end using chunked Transfer Enconding, but that that is not supported on HTTP/2 which is used by the reverse proxy (nginx).

At the moment nginx buffers the chunks before sending them to the browser, which breaks the way the chunks are processed.

With this commit, we use the header X-Accel-Buffering to disable this buffering, which worked in local testing.

We should still add a more clear chunk delimiter so that any other buffering or unknown edge case will not
break the comments function (increase robustness).

Co-authored-by: @ericpp

Fixes #244

Comments for an episode are sent back to the front end using
chunked Transfer Enconding, but that that is not supported
on HTTP/2 which is used by the reverse proxy (nginx).

At the moment nginx buffers the chunks before sending them
to the browser, which breaks the way the chunks are processed.

With this commit, we use the header X-Accel-Buffering to
disable this buffering, which worked in local testing.

We should still add a more clear chunk delimiter so that
any other buffering or unknown edge case will not
break the comments function (increase robustness).

Co-authored-by: ericpp
@daveajones daveajones merged commit 45cce47 into Podcastindex-org:master Apr 1, 2023
@dellagustin
Copy link
Contributor Author

dellagustin commented Apr 13, 2023

Here is the code I used locally to test the web-ui and server behind nginx, based on ericpp@314519a from @ericpp .

./docker/ngingx.conf:

server {
    if ($host = www.podcastindex.org) {
        return 302 https://podcastindex.org$request_uri;
    }

    root /var/www/html;
    index index.html;
    server_name www.podcastindex.org podcastindex.org localhost;
    location / {
        proxy_set_header   X-Forwarded-For $remote_addr;
        proxy_set_header   Host $http_host;
        #proxy_pass         http://127.0.0.1:8000;       # First attempt to serve request as file, then
        proxy_pass         http://host.docker.internal:5001;       # First attempt to serve request as file, then
    }
    listen 80 default_server;
    add_header Strict-Transport-Security "max-age=31536000; includeSubDomains" always;
}

docker-compose.yml:

services:
  nginx:
    image: nginx
    ports: 
      - '8080:80'
    volumes:
      - ./docker/nginx.conf:/etc/nginx/conf.d/default.conf

With that, it can be tested with:

  • yarn install
  • npx webpack --env.file=development --mode development --devtool inline-source-map (the webpack dev server cannot be used for this test, the frontend will be served by the server)
  • yarn start
  • docker-compose up -d
  • open http://localhost:8080/

I don't think this will work on linux. As far as I know, as it does not yet support host.docker.internal (at least not exactly in the same way - see docker/for-linux#264).
See

dellagustin added a commit to podStation/podcastindex-web-ui that referenced this pull request Apr 13, 2023
This commit is a refactoring on the Comments function to improve
robustness for loading partial responses form the comments API.

Once we introduced partial responses (i.e. chunked encoding),
it worked in dev, but failed in production due to buffering in
the reverse proxy (nginx).

This was already solved with
Podcastindex-org#247, but only if
the reverse proxy remains unchanged.

With this refactoring, the implementation would continue to work
even if buffering took place, making it more robust.

For testing, the change introduced in the PR mentioned above was
reverted (temporarily).

For additinal testing instructions, see
Podcastindex-org#247 (comment)
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.

Bug: Comments not loading
2 participants