fix(nvhttp): wrap TLS socket to ensure graceful closure#3077
Merged
ReenigneArcher merged 1 commit intoLizardByte:masterfrom Aug 25, 2024
Merged
fix(nvhttp): wrap TLS socket to ensure graceful closure#3077ReenigneArcher merged 1 commit intoLizardByte:masterfrom
ReenigneArcher merged 1 commit intoLizardByte:masterfrom
Conversation
|
ns6089
reviewed
Aug 24, 2024
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3077 +/- ##
=========================================
- Coverage 9.61% 9.41% -0.20%
=========================================
Files 101 77 -24
Lines 17914 14013 -3901
Branches 8371 6431 -1940
=========================================
- Hits 1722 1319 -403
+ Misses 13321 10080 -3241
+ Partials 2871 2614 -257
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Contributor
|
@cgutman Any migration steps for current users running this behind some reverse proxy or it's mostly transparent? |
Collaborator
Author
|
It's transparent to any reverse proxies. |
KuleRucket
pushed a commit
to KuleRucket/Sunshine
that referenced
this pull request
Oct 9, 2024
This was referenced Jan 18, 2025
c2vi
pushed a commit
to c2vi/Sunshine
that referenced
this pull request
Mar 27, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.


Description
The upstream
SimpleWeb::Server<SimpleWeb::HTTPS>implementation doesn't ensure the TLS session is torn down gracefully which leads to unexpected TCP RST packets from Sunshine to the client when using TLS v1.3 and settingclose_connection_after_response(which NvHTTP does). Depending on the client, this spurious TCP RST can lead to the HTTPS request failing even though all the required data did end up on the wire. TheQSslSocketimplementation in Qt seems to be particularly susceptible to the issue, causing computers to constantly bounce between online and offline.Ideally this would be fixed upstream, but the way that the classes are structured with
SimpleWeb::ServerBasedoesn't provide a straightforward place to insert this code (ServerBasehas no clue about any TLS stuff). Fixing it on our side using a little wrapper class is fairly simple with most of the diff just being renames of the old class to the new one. We already had a class derived fromSimpleWeb::Server<SimpleWeb::HTTPS>, so changing it to derive fromSimpleWeb::ServerBasejust required a few more lines from the upstream constructor ofSimpleWeb::Server<SimpleWeb::HTTPS>.Screenshot
Issues Fixed or Closed
Type of Change
.github/...)Checklist