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

reverseproxy: Ignore context cancel in stream mode #4952

Merged
merged 2 commits into from
Aug 12, 2022
Merged

Conversation

mholt
Copy link
Member

@mholt mholt commented Aug 12, 2022

If FlushInterval is explicitly configured to -1, we assume the user is intending for low-latency streaming, and apparently in those cases clients sometimes disconnect before the request has finished its round-trip with the backend.

This patch ignores the client disconnect in those cases so the request will still be completed without error.

Even though flushInterval() is dynamically computed, for now I'm requiring an explicit configuration of -1 because I don't fully know the implications of this yet. It might be fine, but if there's problems I want them to be opt-in.

We do know this solution at least works though.

Resolves #4922

@mholt mholt added this to the v2.5.3 milestone Aug 12, 2022
@mholt mholt merged commit f5dce84 into master Aug 12, 2022
@mholt mholt deleted the ctxcanceled branch August 12, 2022 19:15
@francislavoie
Copy link
Member

I guess we'll need to document that it must be explicitly configured to get this behaviour, and that relying on the detection stuff (i.e. Content-Type, bidirectional, Content-Length guessing logic) won't get this behaviour.

@mholt
Copy link
Member Author

mholt commented Aug 12, 2022

@francislavoie Yep. I do in the godoc comment in this patch. (I guess we'll want to update Caddyfile docs too.)

WilczynskiT pushed a commit to WilczynskiT/caddy that referenced this pull request Aug 17, 2022
@mholt mholt modified the milestones: v2.6.0-beta.1, v2.6.0 Sep 13, 2022
@wazerstar
Copy link

Has this been implemented? I cannot see any thing about this in change-log on release page.

I'm testing some streaming and using hdhomerun, when client leaves,stops,change channel without using "flush_interval -1"

The outcome is this
{"resumed":false,"version":772,"cipher_suite":4865,"proto":"h2","server_name":"url"}},"error":"reading: context canceled"}

then if I Add flush_interval -1 to reverse_proxy

outcome will become this
{"resumed":false,"version":772,"cipher_suite":4865,"proto":"h2","server_name":"url"}},"error":"writing: client disconnected"}

@mholt
Copy link
Member Author

mholt commented Jul 27, 2023

Yes, it was released; I'm not sure why it didn't make it into the release notes, but it's definitely released with v2.6.

If you open a new issue with instructions to minimally reproduce the behavior we can look into it.

mholt added a commit that referenced this pull request Oct 28, 2024
Sponsor request is to terminate streaming connections with backend when client leaves, but wants immediate flushing.

See #4922 and #4952.
@mholt
Copy link
Member Author

mholt commented Oct 28, 2024

This behavior might possibly be changing, by splitting it out into a separate config option other than flush_interval, since there's really two things going on there and that's basically a bug.

See 8c9e87d -- for now I've implemented a new option, ignore_client_gone, to enable this behavior that until now only required setting flush_interval to -1. But flush_interval should really just change the flush interval, and the new option should configure the client disconnect handling.

mholt added a commit that referenced this pull request Nov 12, 2024
…eam mode

i.e. Revert commit f5dce84

Two years ago, the patch in #4952 was a seemingly necessary way to fix an issue (sort of an edge case), but it broke other more common use cases (see #6666).

Now, as of #6669, it seems like the original issue can no longer be replicated, so we are reverting that patch, because it was incorrect anyway.

If it turns out the original issue returns, a more proper patch may be in #6669 (even if used as a baseline for a future fix). A potential future fix could be an opt-in setting.
mholt added a commit that referenced this pull request Nov 12, 2024
…eam mode

i.e. Revert commit f5dce84

Two years ago, the patch in #4952 was a seemingly necessary way to fix an issue (sort of an edge case), but it broke other more common use cases (see #6666).

Now, as of #6669, it seems like the original issue can no longer be replicated, so we are reverting that patch, because it was incorrect anyway.

If it turns out the original issue returns, a more proper patch may be in #6669 (even if used as a baseline for a future fix). A potential future fix could be an opt-in setting.
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.

caddyserver-reverseproxy + ffmpeg streaming causes invalid files/connection state
3 participants