fix(nvhttp): wrap TLS socket to ensure graceful closure by cgutman · Pull Request #3077 · LizardByte/Sunshine

cgutman · 2024-08-24T07:37:30Z

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 setting close_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. The QSslSocket implementation 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::ServerBase doesn't provide a straightforward place to insert this code (ServerBase has 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 from SimpleWeb::Server<SimpleWeb::HTTPS>, so changing it to derive from SimpleWeb::ServerBase just required a few more lines from the upstream constructor of SimpleWeb::Server<SimpleWeb::HTTPS>.

Screenshot

Issues Fixed or Closed

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Dependency update (updates to dependencies)
Documentation update (changes to documentation)
Repository update (changes to repository files, e.g. .github/...)

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

sonarqubecloud · 2024-08-24T07:44:56Z

Quality Gate failed

Failed conditions
5.1% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

codecov · 2024-08-24T07:47:47Z

Codecov Report

Attention: Patch coverage is 4.76190% with 20 lines in your changes missing coverage. Please review.

Project coverage is 9.41%. Comparing base (c9f853d) to head (81c012b).
Report is 121 commits behind head on master.

Files with missing lines	Patch %	Lines
src/nvhttp.cpp	4.76%	7 Missing and 13 partials ⚠️

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

Flag	Coverage Δ
Linux	`?`
Windows	`4.95% <0.00%> (-0.01%)`	⬇️
macOS-12	`10.36% <4.76%> (-0.05%)`	⬇️
macOS-13	`10.27% <4.76%> (-0.05%)`	⬇️
macOS-14	`10.55% <4.76%> (-0.05%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
src/nvhttp.cpp	`0.89% <4.76%> (-0.32%)`	⬇️

... and 54 files with indirect coverage changes

---- 🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

Hazer · 2024-08-25T05:10:05Z

@cgutman Any migration steps for current users running this behind some reverse proxy or it's mostly transparent?

cgutman · 2024-08-25T19:22:26Z

It's transparent to any reverse proxies.

)

cgutman · 2024-08-24T07:37:30Z

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 setting close_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. The QSslSocket implementation 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::ServerBase doesn't provide a straightforward place to insert this code (ServerBase has 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 from SimpleWeb::Server<SimpleWeb::HTTPS>, so changing it to derive from SimpleWeb::ServerBase just required a few more lines from the upstream constructor of SimpleWeb::Server<SimpleWeb::HTTPS>.

Screenshot

Issues Fixed or Closed

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Dependency update (updates to dependencies)
Documentation update (changes to documentation)
Repository update (changes to repository files, e.g. .github/...)

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

sonarqubecloud · 2024-08-24T07:44:56Z

Quality Gate failed

Failed conditions
5.1% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

codecov · 2024-08-24T07:47:47Z

Codecov Report

Attention: Patch coverage is 4.76190% with 20 lines in your changes missing coverage. Please review.

Project coverage is 9.41%. Comparing base (c9f853d) to head (81c012b).
Report is 121 commits behind head on master.

Files with missing lines	Patch %	Lines
src/nvhttp.cpp	4.76%	7 Missing and 13 partials ⚠️

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

Flag	Coverage Δ
Linux	`?`
Windows	`4.95% <0.00%> (-0.01%)`	⬇️
macOS-12	`10.36% <4.76%> (-0.05%)`	⬇️
macOS-13	`10.27% <4.76%> (-0.05%)`	⬇️
macOS-14	`10.55% <4.76%> (-0.05%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
src/nvhttp.cpp	`0.89% <4.76%> (-0.32%)`	⬇️

... and 54 files with indirect coverage changes

---- 🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

Hazer · 2024-08-25T05:10:05Z

@cgutman Any migration steps for current users running this behind some reverse proxy or it's mostly transparent?

cgutman · 2024-08-25T19:22:26Z

It's transparent to any reverse proxies.

)

cgutman · 2024-08-24T07:37:30Z

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 setting close_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. The QSslSocket implementation 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::ServerBase doesn't provide a straightforward place to insert this code (ServerBase has 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 from SimpleWeb::Server<SimpleWeb::HTTPS>, so changing it to derive from SimpleWeb::ServerBase just required a few more lines from the upstream constructor of SimpleWeb::Server<SimpleWeb::HTTPS>.

Screenshot

Issues Fixed or Closed

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Dependency update (updates to dependencies)
Documentation update (changes to documentation)
Repository update (changes to repository files, e.g. .github/...)

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

sonarqubecloud · 2024-08-24T07:44:56Z

Quality Gate failed

Failed conditions
5.1% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

codecov · 2024-08-24T07:47:47Z

Codecov Report

Attention: Patch coverage is 4.76190% with 20 lines in your changes missing coverage. Please review.

Project coverage is 9.41%. Comparing base (c9f853d) to head (81c012b).
Report is 121 commits behind head on master.

Files with missing lines	Patch %	Lines
src/nvhttp.cpp	4.76%	7 Missing and 13 partials ⚠️

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

Flag	Coverage Δ
Linux	`?`
Windows	`4.95% <0.00%> (-0.01%)`	⬇️
macOS-12	`10.36% <4.76%> (-0.05%)`	⬇️
macOS-13	`10.27% <4.76%> (-0.05%)`	⬇️
macOS-14	`10.55% <4.76%> (-0.05%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
src/nvhttp.cpp	`0.89% <4.76%> (-0.32%)`	⬇️

... and 54 files with indirect coverage changes

---- 🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

Hazer · 2024-08-25T05:10:05Z

@cgutman Any migration steps for current users running this behind some reverse proxy or it's mostly transparent?

cgutman · 2024-08-25T19:22:26Z

It's transparent to any reverse proxies.

)

cgutman · 2024-08-24T07:37:30Z

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 setting close_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. The QSslSocket implementation 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::ServerBase doesn't provide a straightforward place to insert this code (ServerBase has 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 from SimpleWeb::Server<SimpleWeb::HTTPS>, so changing it to derive from SimpleWeb::ServerBase just required a few more lines from the upstream constructor of SimpleWeb::Server<SimpleWeb::HTTPS>.

Screenshot

Issues Fixed or Closed

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Dependency update (updates to dependencies)
Documentation update (changes to documentation)
Repository update (changes to repository files, e.g. .github/...)

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

sonarqubecloud · 2024-08-24T07:44:56Z

Quality Gate failed

Failed conditions
5.1% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

codecov · 2024-08-24T07:47:47Z

Codecov Report

Attention: Patch coverage is 4.76190% with 20 lines in your changes missing coverage. Please review.

Project coverage is 9.41%. Comparing base (c9f853d) to head (81c012b).
Report is 121 commits behind head on master.

Files with missing lines	Patch %	Lines
src/nvhttp.cpp	4.76%	7 Missing and 13 partials ⚠️

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

Flag	Coverage Δ
Linux	`?`
Windows	`4.95% <0.00%> (-0.01%)`	⬇️
macOS-12	`10.36% <4.76%> (-0.05%)`	⬇️
macOS-13	`10.27% <4.76%> (-0.05%)`	⬇️
macOS-14	`10.55% <4.76%> (-0.05%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
src/nvhttp.cpp	`0.89% <4.76%> (-0.32%)`	⬇️

... and 54 files with indirect coverage changes

---- 🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

Hazer · 2024-08-25T05:10:05Z

@cgutman Any migration steps for current users running this behind some reverse proxy or it's mostly transparent?

cgutman · 2024-08-25T19:22:26Z

It's transparent to any reverse proxies.

)

cgutman · 2024-08-24T07:37:30Z

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 setting close_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. The QSslSocket implementation 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::ServerBase doesn't provide a straightforward place to insert this code (ServerBase has 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 from SimpleWeb::Server<SimpleWeb::HTTPS>, so changing it to derive from SimpleWeb::ServerBase just required a few more lines from the upstream constructor of SimpleWeb::Server<SimpleWeb::HTTPS>.

Screenshot

Issues Fixed or Closed

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Dependency update (updates to dependencies)
Documentation update (changes to documentation)
Repository update (changes to repository files, e.g. .github/...)

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

sonarqubecloud · 2024-08-24T07:44:56Z

Quality Gate failed

Failed conditions
5.1% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

codecov · 2024-08-24T07:47:47Z

Codecov Report

Attention: Patch coverage is 4.76190% with 20 lines in your changes missing coverage. Please review.

Project coverage is 9.41%. Comparing base (c9f853d) to head (81c012b).
Report is 121 commits behind head on master.

Files with missing lines	Patch %	Lines
src/nvhttp.cpp	4.76%	7 Missing and 13 partials ⚠️

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

Flag	Coverage Δ
Linux	`?`
Windows	`4.95% <0.00%> (-0.01%)`	⬇️
macOS-12	`10.36% <4.76%> (-0.05%)`	⬇️
macOS-13	`10.27% <4.76%> (-0.05%)`	⬇️
macOS-14	`10.55% <4.76%> (-0.05%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
src/nvhttp.cpp	`0.89% <4.76%> (-0.32%)`	⬇️

... and 54 files with indirect coverage changes

---- 🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

Hazer · 2024-08-25T05:10:05Z

@cgutman Any migration steps for current users running this behind some reverse proxy or it's mostly transparent?

cgutman · 2024-08-25T19:22:26Z

It's transparent to any reverse proxies.

)

cgutman · 2024-08-24T07:37:30Z

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 setting close_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. The QSslSocket implementation 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::ServerBase doesn't provide a straightforward place to insert this code (ServerBase has 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 from SimpleWeb::Server<SimpleWeb::HTTPS>, so changing it to derive from SimpleWeb::ServerBase just required a few more lines from the upstream constructor of SimpleWeb::Server<SimpleWeb::HTTPS>.

Screenshot

Issues Fixed or Closed

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Dependency update (updates to dependencies)
Documentation update (changes to documentation)
Repository update (changes to repository files, e.g. .github/...)

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

sonarqubecloud · 2024-08-24T07:44:56Z

Quality Gate failed

Failed conditions
5.1% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

codecov · 2024-08-24T07:47:47Z

Codecov Report

Attention: Patch coverage is 4.76190% with 20 lines in your changes missing coverage. Please review.

Project coverage is 9.41%. Comparing base (c9f853d) to head (81c012b).
Report is 121 commits behind head on master.

Files with missing lines	Patch %	Lines
src/nvhttp.cpp	4.76%	7 Missing and 13 partials ⚠️

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

Flag	Coverage Δ
Linux	`?`
Windows	`4.95% <0.00%> (-0.01%)`	⬇️
macOS-12	`10.36% <4.76%> (-0.05%)`	⬇️
macOS-13	`10.27% <4.76%> (-0.05%)`	⬇️
macOS-14	`10.55% <4.76%> (-0.05%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
src/nvhttp.cpp	`0.89% <4.76%> (-0.32%)`	⬇️

... and 54 files with indirect coverage changes

---- 🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

Hazer · 2024-08-25T05:10:05Z

@cgutman Any migration steps for current users running this behind some reverse proxy or it's mostly transparent?

cgutman · 2024-08-25T19:22:26Z

It's transparent to any reverse proxies.

)

cgutman · 2024-08-24T07:37:30Z

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 setting close_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. The QSslSocket implementation 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::ServerBase doesn't provide a straightforward place to insert this code (ServerBase has 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 from SimpleWeb::Server<SimpleWeb::HTTPS>, so changing it to derive from SimpleWeb::ServerBase just required a few more lines from the upstream constructor of SimpleWeb::Server<SimpleWeb::HTTPS>.

Screenshot

Issues Fixed or Closed

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Dependency update (updates to dependencies)
Documentation update (changes to documentation)
Repository update (changes to repository files, e.g. .github/...)

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

sonarqubecloud · 2024-08-24T07:44:56Z

Quality Gate failed

Failed conditions
5.1% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

codecov · 2024-08-24T07:47:47Z

Codecov Report

Attention: Patch coverage is 4.76190% with 20 lines in your changes missing coverage. Please review.

Project coverage is 9.41%. Comparing base (c9f853d) to head (81c012b).
Report is 121 commits behind head on master.

Files with missing lines	Patch %	Lines
src/nvhttp.cpp	4.76%	7 Missing and 13 partials ⚠️

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

Flag	Coverage Δ
Linux	`?`
Windows	`4.95% <0.00%> (-0.01%)`	⬇️
macOS-12	`10.36% <4.76%> (-0.05%)`	⬇️
macOS-13	`10.27% <4.76%> (-0.05%)`	⬇️
macOS-14	`10.55% <4.76%> (-0.05%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
src/nvhttp.cpp	`0.89% <4.76%> (-0.32%)`	⬇️

... and 54 files with indirect coverage changes

---- 🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

Hazer · 2024-08-25T05:10:05Z

@cgutman Any migration steps for current users running this behind some reverse proxy or it's mostly transparent?

cgutman · 2024-08-25T19:22:26Z

It's transparent to any reverse proxies.

)

cgutman · 2024-08-24T07:37:30Z

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 setting close_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. The QSslSocket implementation 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::ServerBase doesn't provide a straightforward place to insert this code (ServerBase has 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 from SimpleWeb::Server<SimpleWeb::HTTPS>, so changing it to derive from SimpleWeb::ServerBase just required a few more lines from the upstream constructor of SimpleWeb::Server<SimpleWeb::HTTPS>.

Screenshot

Issues Fixed or Closed

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Dependency update (updates to dependencies)
Documentation update (changes to documentation)
Repository update (changes to repository files, e.g. .github/...)

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

sonarqubecloud · 2024-08-24T07:44:56Z

Quality Gate failed

Failed conditions
5.1% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

codecov · 2024-08-24T07:47:47Z

Codecov Report

Attention: Patch coverage is 4.76190% with 20 lines in your changes missing coverage. Please review.

Project coverage is 9.41%. Comparing base (c9f853d) to head (81c012b).
Report is 121 commits behind head on master.

Files with missing lines	Patch %	Lines
src/nvhttp.cpp	4.76%	7 Missing and 13 partials ⚠️

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

Flag	Coverage Δ
Linux	`?`
Windows	`4.95% <0.00%> (-0.01%)`	⬇️
macOS-12	`10.36% <4.76%> (-0.05%)`	⬇️
macOS-13	`10.27% <4.76%> (-0.05%)`	⬇️
macOS-14	`10.55% <4.76%> (-0.05%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
src/nvhttp.cpp	`0.89% <4.76%> (-0.32%)`	⬇️

... and 54 files with indirect coverage changes

---- 🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

Hazer · 2024-08-25T05:10:05Z

@cgutman Any migration steps for current users running this behind some reverse proxy or it's mostly transparent?

cgutman · 2024-08-25T19:22:26Z

It's transparent to any reverse proxies.

)

fix(nvhttp): wrap TLS socket to ensure graceful closure
81c012b

cgutman added this to the stable release milestone Aug 24, 2024

ns6089 reviewed Aug 24, 2024
View reviewed changes

Comment threadsrc/nvhttp.cpp

ReenigneArcher merged commit f048510 into LizardByte:masterAug 25, 2024

KuleRucket pushed a commit to KuleRucket/Sunshine that referenced this pull request Oct 9, 2024
fix(nvhttp): wrap TLS socket to ensure graceful closure (LizardByte#3077
495ffbb
)

ReenigneArcher mentioned this pull request Jan 22, 2025
chore: Update dev.lizardbyte.app.Sunshine to v2025.122.141614 flathub/dev.lizardbyte.app.Sunshine#185
Merged

c2vi pushed a commit to c2vi/Sunshine that referenced this pull request Mar 27, 2025
fix(nvhttp): wrap TLS socket to ensure graceful closure (LizardByte#3077
0ba390d
)

fix(nvhttp): wrap TLS socket to ensure graceful closure
81c012b

cgutman added this to the stable release milestone Aug 24, 2024

ns6089 reviewed Aug 24, 2024
View reviewed changes

Comment threadsrc/nvhttp.cpp

ReenigneArcher merged commit f048510 into LizardByte:masterAug 25, 2024

KuleRucket pushed a commit to KuleRucket/Sunshine that referenced this pull request Oct 9, 2024
fix(nvhttp): wrap TLS socket to ensure graceful closure (LizardByte#3077
495ffbb
)

ReenigneArcher mentioned this pull request Jan 22, 2025
chore: Update dev.lizardbyte.app.Sunshine to v2025.122.141614 flathub/dev.lizardbyte.app.Sunshine#185
Merged

c2vi pushed a commit to c2vi/Sunshine that referenced this pull request Mar 27, 2025
fix(nvhttp): wrap TLS socket to ensure graceful closure (LizardByte#3077
0ba390d
)

fix(nvhttp): wrap TLS socket to ensure graceful closure
81c012b

cgutman added this to the stable release milestone Aug 24, 2024

ns6089 reviewed Aug 24, 2024
View reviewed changes

Comment threadsrc/nvhttp.cpp

ReenigneArcher merged commit f048510 into LizardByte:masterAug 25, 2024

KuleRucket pushed a commit to KuleRucket/Sunshine that referenced this pull request Oct 9, 2024
fix(nvhttp): wrap TLS socket to ensure graceful closure (LizardByte#3077
495ffbb
)

ReenigneArcher mentioned this pull request Jan 22, 2025
chore: Update dev.lizardbyte.app.Sunshine to v2025.122.141614 flathub/dev.lizardbyte.app.Sunshine#185
Merged

c2vi pushed a commit to c2vi/Sunshine that referenced this pull request Mar 27, 2025
fix(nvhttp): wrap TLS socket to ensure graceful closure (LizardByte#3077
0ba390d
)

fix(nvhttp): wrap TLS socket to ensure graceful closure
81c012b

cgutman added this to the stable release milestone Aug 24, 2024

ns6089 reviewed Aug 24, 2024
View reviewed changes

Comment threadsrc/nvhttp.cpp

ReenigneArcher merged commit f048510 into LizardByte:masterAug 25, 2024

KuleRucket pushed a commit to KuleRucket/Sunshine that referenced this pull request Oct 9, 2024
fix(nvhttp): wrap TLS socket to ensure graceful closure (LizardByte#3077
495ffbb
)

ReenigneArcher mentioned this pull request Jan 22, 2025
chore: Update dev.lizardbyte.app.Sunshine to v2025.122.141614 flathub/dev.lizardbyte.app.Sunshine#185
Merged

c2vi pushed a commit to c2vi/Sunshine that referenced this pull request Mar 27, 2025
fix(nvhttp): wrap TLS socket to ensure graceful closure (LizardByte#3077
0ba390d
)

fix(nvhttp): wrap TLS socket to ensure graceful closure
81c012b

cgutman added this to the stable release milestone Aug 24, 2024

ns6089 reviewed Aug 24, 2024
View reviewed changes

Comment threadsrc/nvhttp.cpp

ReenigneArcher merged commit f048510 into LizardByte:masterAug 25, 2024

KuleRucket pushed a commit to KuleRucket/Sunshine that referenced this pull request Oct 9, 2024
fix(nvhttp): wrap TLS socket to ensure graceful closure (LizardByte#3077
495ffbb
)

ReenigneArcher mentioned this pull request Jan 22, 2025
chore: Update dev.lizardbyte.app.Sunshine to v2025.122.141614 flathub/dev.lizardbyte.app.Sunshine#185
Merged

c2vi pushed a commit to c2vi/Sunshine that referenced this pull request Mar 27, 2025
fix(nvhttp): wrap TLS socket to ensure graceful closure (LizardByte#3077
0ba390d
)

fix(nvhttp): wrap TLS socket to ensure graceful closure
81c012b

cgutman added this to the stable release milestone Aug 24, 2024

ns6089 reviewed Aug 24, 2024
View reviewed changes

Comment threadsrc/nvhttp.cpp

ReenigneArcher merged commit f048510 into LizardByte:masterAug 25, 2024

KuleRucket pushed a commit to KuleRucket/Sunshine that referenced this pull request Oct 9, 2024
fix(nvhttp): wrap TLS socket to ensure graceful closure (LizardByte#3077
495ffbb
)

ReenigneArcher mentioned this pull request Jan 22, 2025
chore: Update dev.lizardbyte.app.Sunshine to v2025.122.141614 flathub/dev.lizardbyte.app.Sunshine#185
Merged

c2vi pushed a commit to c2vi/Sunshine that referenced this pull request Mar 27, 2025
fix(nvhttp): wrap TLS socket to ensure graceful closure (LizardByte#3077
0ba390d
)

fix(nvhttp): wrap TLS socket to ensure graceful closure
81c012b

cgutman added this to the stable release milestone Aug 24, 2024

ns6089 reviewed Aug 24, 2024
View reviewed changes

Comment threadsrc/nvhttp.cpp

ReenigneArcher merged commit f048510 into LizardByte:masterAug 25, 2024

KuleRucket pushed a commit to KuleRucket/Sunshine that referenced this pull request Oct 9, 2024
fix(nvhttp): wrap TLS socket to ensure graceful closure (LizardByte#3077
495ffbb
)

ReenigneArcher mentioned this pull request Jan 22, 2025
chore: Update dev.lizardbyte.app.Sunshine to v2025.122.141614 flathub/dev.lizardbyte.app.Sunshine#185
Merged

c2vi pushed a commit to c2vi/Sunshine that referenced this pull request Mar 27, 2025
fix(nvhttp): wrap TLS socket to ensure graceful closure (LizardByte#3077
0ba390d
)

fix(nvhttp): wrap TLS socket to ensure graceful closure
81c012b

cgutman added this to the stable release milestone Aug 24, 2024

ns6089 reviewed Aug 24, 2024
View reviewed changes

Comment threadsrc/nvhttp.cpp

ReenigneArcher merged commit f048510 into LizardByte:masterAug 25, 2024

KuleRucket pushed a commit to KuleRucket/Sunshine that referenced this pull request Oct 9, 2024
fix(nvhttp): wrap TLS socket to ensure graceful closure (LizardByte#3077
495ffbb
)

ReenigneArcher mentioned this pull request Jan 22, 2025
chore: Update dev.lizardbyte.app.Sunshine to v2025.122.141614 flathub/dev.lizardbyte.app.Sunshine#185
Merged

c2vi pushed a commit to c2vi/Sunshine that referenced this pull request Mar 27, 2025
fix(nvhttp): wrap TLS socket to ensure graceful closure (LizardByte#3077
0ba390d
)

Uh oh!

Uh oh!

Conversation

cgutman commented Aug 24, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Screenshot

Issues Fixed or Closed

Type of Change

Checklist

Uh oh!

sonarqubecloudBot commented Aug 24, 2024

Quality Gate failed

Uh oh!

Uh oh!

codecovBot commented Aug 24, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Hazer commented Aug 25, 2024

Uh oh!

cgutman commented Aug 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Uh oh!

Conversation

cgutman commented Aug 24, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Screenshot

Issues Fixed or Closed

Type of Change

Checklist

Uh oh!

sonarqubecloudBot commented Aug 24, 2024

Quality Gate failed

Uh oh!

Uh oh!

codecovBot commented Aug 24, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Hazer commented Aug 25, 2024

Uh oh!

cgutman commented Aug 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Uh oh!

Conversation

cgutman commented Aug 24, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Screenshot

Issues Fixed or Closed

Type of Change

Checklist

Uh oh!

sonarqubecloudBot commented Aug 24, 2024

Quality Gate failed

Uh oh!

Uh oh!

codecovBot commented Aug 24, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

cgutman commented Aug 24, 2024•
edited
Loading

codecovBot commented Aug 24, 2024•
edited
Loading

cgutman commented Aug 24, 2024•
edited
Loading

codecovBot commented Aug 24, 2024•
edited
Loading

cgutman commented Aug 24, 2024•
edited
Loading

codecovBot commented Aug 24, 2024•
edited
Loading

cgutman commented Aug 24, 2024•
edited
Loading

codecovBot commented Aug 24, 2024•
edited
Loading