Implement TCP Keep-Alive for WinHttpHandler - #44889

Merged
antonfirsov merged 7 commits into
dotnet:masterfrom
antonfirsov:WinHttp-TcpKeepalive
Nov 25, 2020
Merged

Implement TCP Keep-Alive for WinHttpHandler#44889
antonfirsov merged 7 commits into
dotnet:masterfrom
antonfirsov:WinHttp-TcpKeepalive

Conversation

@antonfirsov

@antonfirsovantonfirsov commented Nov 18, 2020

Copy link
Copy Markdown
Contributor

This implements the API proposal from #44025 (comment) except the [SupportedOSPlatform("windows10.0.2004")] bits. I prefer to handle that separately in a follow-up PR, since it doesn't seem to be a a functional concern, but rather focuses on managing build targets.

I decided to handle compatibility in a way consistent with other properties:
WinHttpException will be thrown, if OS doesn't support the property.

Contributes to #44025.

/cc @geoffkizer@wfurt@alnikola

@Dotnet-GitSync-Bot

Dotnet-GitSync-Bot commented Nov 18, 2020

Copy link
Copy Markdown
Collaborator

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost

ghost commented Nov 18, 2020

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Details

Issue Details
Description:

This implements the API proposal from #44025 (comment) except the [SupportedOSPlatform("windows10.0.2004")] bits. I prefer to handle that separately in a follow-up PR, since it doesn't seem to be a a functional concern, but rather focuses on managing build targets.

I decided to handle compatibility in a way consistent with other properties:
WinHttpException will be thrown, if OS doesn't support the property.

Contributes to #44025.

/cc @geoffkizer@wfurt@alnikola

Author:antonfirsov
Assignees:-
Labels:

area-System.Net, new-api-needs-documentation

Milestone:-

geoffkizer
geoffkizer reviewed Nov 18, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs Outdated
}
}

public bool TcpKeepAliveEnabled { get; set; }

@geoffkizergeoffkizerNov 18, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the set here call CheckDisposedOrStarted?

@stephentoubstephentoubNov 18, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default here is false? I'm just questioning that because the comment earlier says "The default settings when a TCP socket is initialized sets the keep-alive timeout to 2 hours and the keep-alive interval to 1 second" which suggests the default here should actually be true?

@antonfirsovantonfirsovNov 19, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default settings when a TCP socket is initialized sets the keep-alive timeout to 2 hours and the keep-alive interval to 1 second

The same docs also state that :

The SO_KEEPALIVE option for a socket is disabled (set to FALSE) by default. [...] When this socket option is enabled, the TCP stack sends keep-alive packets when no data or acknowledgement packets have been received for the connection within an interval.

From this I concluded that if SO_KEEPALIVE or SIO_KEEPALIVE_VALS are left unchanged, no keepalive packets gonna be sent. We may want to double check with the winsock team since the wording is kinda contradictional.

@stephentoubstephentoubNov 19, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's double-check, and then either update the implementation or the comment :)

@antonfirsovantonfirsovNov 19, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The defaults is false. This discussion resulted in an update of the docs ;)

MicrosoftDocs/win32#635

@halter73halter73Nov 19, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I never see TCP keep-alive packets in HTTP network traces I get from customers, and that would be pretty obvious with the 1 second interval. I know we decided we're sticking with winsock defaults and violating RFC, but the RFC does say TCP keep-alives should default off in no uncertain terms:

 Implementors MAY include "keep-alives" in their TCP
implementations, although this practice is not universally
accepted. If keep-alives are included, the application MUST
be able to turn them on or off for each TCP connection, and
they MUST default to off.

https://tools.ietf.org/html/rfc1122#page-101

I'm glad the winsock defaults at least align with the RFC on this part.

geoffkizer
geoffkizer reviewed Nov 18, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http.WinHttpHandler/tests/UnitTests/APICallHistory.cs Outdated

public static List<IntPtr> WinHttpOptionClientCertContext { get { return winHttpOptionClientCertContextList; } }

public static (uint KeepAliveTime, uint KeepAliveInterval)? TcpKeepaliveOptions { get; set; }

@geoffkizergeoffkizerNov 18, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the other properties here are named WinHttpOption*, should we do the same for this one?

geoffkizer
geoffkizer approved these changes Nov 18, 2020
View reviewed changes

@geoffkizergeoffkizer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple small issues above, otherwise LGTM

stephentoub
stephentoub reviewed Nov 18, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs Outdated
keepaliveinterval = (uint)_tcpKeepAliveInterval.TotalMilliseconds,
keepalivetime = (uint)_tcpKeepAliveTime.TotalMilliseconds
};
void* ptr = &tcpKeepalive;

@stephentoubstephentoubNov 18, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this local seems unnecessary, and you can just use (IntPtr)&tcpKeepalive below.

@runfoapprunfoappBot mentioned this pull request Nov 18, 2020
Closed
@antonfirsov

This comment has been minimized.

Sign in to view
@azure-pipelines

This comment has been minimized.

Sign in to view
@antonfirsov

antonfirsov commented Nov 19, 2020•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@geoffkizer I think all finding have been addressed, wondering if it's worth to wait for a few more approvals for this.

@geoffkizer

geoffkizer commented Nov 19, 2020

Copy link
Copy Markdown
Contributor

LGTM

@antonfirsov

antonfirsov commented Nov 23, 2020•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@geoffkizer@stephentoub what about documentation? Should I add xmldoc right in the PR, or is it sufficient to have the new-api-needs-documentation label? Do we apply the same rules for NuGet libraries as for the rest?

@stephentoub

stephentoub commented Nov 23, 2020

Copy link
Copy Markdown
Member

Should I add xmldoc right in the PR

Yup, thanks.
cc: @carlossanlop

@antonfirsovantonfirsov mentioned this pull request Nov 23, 2020
Merged
scalablecory
scalablecory approved these changes Nov 24, 2020
View reviewed changes

@scalablecoryscalablecory left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to improve validation, otherwise lgtm.

Comment threadsrc/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs
@antonfirsov

antonfirsov commented Nov 24, 2020

Copy link
Copy Markdown
ContributorAuthor

I've added the docs, can someone double-check the them before we merge?

@geoffkizer

geoffkizer commented Nov 24, 2020

Copy link
Copy Markdown
Contributor

LGTM

@antonfirsov
antonfirsov merged commit 837785f into dotnet:masterNov 25, 2020
@ManickaPManickaP mentioned this pull request Nov 26, 2020
Closed
@ghostghost locked as resolved and limited conversation to collaborators Dec 25, 2020
@karelzkarelz added this to the 6.0.0 milestone Jan 26, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@halter73halter73halter73 left review comments
@stephentoubstephentoubstephentoub left review comments
+2 more reviewers
@scalablecoryscalablecoryscalablecory approved these changes
@geoffkizergeoffkizergeoffkizer approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Netnew-api-needs-documentation

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

7 participants

@antonfirsov@Dotnet-GitSync-Bot@geoffkizer@stephentoub@halter73@scalablecory@karelz
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Implement TCP Keep-Alive for WinHttpHandler - #44889
Merged
antonfirsov merged 7 commits into
dotnet:masterfrom 
antonfirsov:WinHttp-TcpKeepalive
Nov 25, 2020
Merged
Implement TCP Keep-Alive for WinHttpHandler#44889
antonfirsov merged 7 commits into
dotnet:masterfrom 
antonfirsov:WinHttp-TcpKeepalive
Conversation
@antonfirsov
@antonfirsovantonfirsov
commented
Nov 18, 2020

Copy link


Copy Markdown

Contributor
This implements the API proposal from #44025 (comment) except the  [SupportedOSPlatform("windows10.0.2004")] bits. I prefer to handle that separately in a follow-up PR, since it doesn't seem to be a a functional concern, but rather focuses on managing build targets.
I decided to handle compatibility in a way consistent with other properties:
WinHttpException will be thrown, if OS doesn't support the property.
Contributes to #44025.
/cc @geoffkizer@wfurt@alnikola
@Dotnet-GitSync-Bot
Dotnet-GitSync-Bot
commented
Nov 18, 2020

Copy link


Copy Markdown

Collaborator
Note regarding the new-api-needs-documentation label:
This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.
@ghost
ghost
commented
Nov 18, 2020

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/ncl

See info in area-owners.md if you want to be subscribed.
Details
Issue Details
Description:
This implements the API proposal from #44025 (comment) except the  [SupportedOSPlatform("windows10.0.2004")] bits. I prefer to handle that separately in a follow-up PR, since it doesn't seem to be a a functional concern, but rather focuses on managing build targets.
I decided to handle compatibility in a way consistent with other properties:
WinHttpException will be thrown, if OS doesn't support the property.
Contributes to #44025.
/cc @geoffkizer@wfurt@alnikola
Author:antonfirsov
Assignees:-
Labels:
area-System.Net, new-api-needs-documentation
Milestone:-
geoffkizer
geoffkizer
reviewed
Nov 18, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs
Outdated

 }
 }

 public bool TcpKeepAliveEnabled { get; set; }
@geoffkizergeoffkizerNov 18, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the set here call CheckDisposedOrStarted?
@stephentoubstephentoubNov 18, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default here is false? I'm just questioning that because the comment earlier says "The default settings when a TCP socket is initialized sets the keep-alive timeout to 2 hours and the keep-alive interval to 1 second" which suggests the default here should actually be true?
@antonfirsovantonfirsovNov 19, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default settings when a TCP socket is initialized sets the keep-alive timeout to 2 hours and the keep-alive interval to 1 second
The same docs also state that :
The SO_KEEPALIVE option for a socket is disabled (set to FALSE) by default. [...] When this socket option is enabled, the TCP stack sends keep-alive packets when no data or acknowledgement packets have been received for the connection within an interval.
From this I concluded that if SO_KEEPALIVE  or SIO_KEEPALIVE_VALS are left unchanged, no keepalive packets gonna be sent. We may want to double check with the winsock team since the wording is kinda contradictional.
@stephentoubstephentoubNov 19, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's double-check, and then either update the implementation or the comment :)
@antonfirsovantonfirsovNov 19, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The defaults is false. This discussion resulted in an update of the docs ;)
MicrosoftDocs/win32#635
@halter73halter73Nov 19, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I never see TCP keep-alive packets in HTTP network traces I get from customers, and that would be pretty obvious with the 1 second interval. I know we decided we're sticking with winsock defaults and violating RFC, but the RFC does say TCP keep-alives should default off in no uncertain terms:
 Implementors MAY include "keep-alives" in their TCP
implementations, although this practice is not universally
accepted. If keep-alives are included, the application MUST
be able to turn them on or off for each TCP connection, and
they MUST default to off.

https://tools.ietf.org/html/rfc1122#page-101
I'm glad the winsock defaults at least align with the RFC on this part.
geoffkizer
geoffkizer
reviewed
Nov 18, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http.WinHttpHandler/tests/UnitTests/APICallHistory.cs
Outdated


 public static List<IntPtr> WinHttpOptionClientCertContext { get { return winHttpOptionClientCertContextList; } }

 public static (uint KeepAliveTime, uint KeepAliveInterval)? TcpKeepaliveOptions { get; set; }
@geoffkizergeoffkizerNov 18, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the other properties here are named WinHttpOption*, should we do the same for this one?
geoffkizer
geoffkizer
approved these changes
Nov 18, 2020
View reviewed changes
@geoffkizergeoffkizer
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple small issues above, otherwise LGTM
stephentoub
stephentoub
reviewed
Nov 18, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs
Outdated

 keepaliveinterval = (uint)_tcpKeepAliveInterval.TotalMilliseconds,
 keepalivetime = (uint)_tcpKeepAliveTime.TotalMilliseconds
 };
 void* ptr = &tcpKeepalive;
@stephentoubstephentoubNov 18, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this local seems unnecessary, and you can just use (IntPtr)&tcpKeepalive below.
@runfoapprunfoappBot
mentioned this pull request
Nov 18, 2020

Closed

@antonfirsov

This comment has been minimized.

Sign in to view
@azure-pipelines

This comment has been minimized.

Sign in to view
@antonfirsov
antonfirsov
commented
Nov 19, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@geoffkizer I think all finding have been addressed, wondering if it's worth to wait for a few more approvals for this.
@geoffkizer
geoffkizer
commented
Nov 19, 2020

Copy link


Copy Markdown

Contributor
LGTM
@antonfirsov
antonfirsov
commented
Nov 23, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@geoffkizer@stephentoub what about documentation? Should I add xmldoc right in the PR, or is it sufficient to have the new-api-needs-documentation label? Do we apply the same rules for NuGet libraries as for the rest?
@stephentoub
stephentoub
commented
Nov 23, 2020

Copy link


Copy Markdown

Member
Should I add xmldoc right in the PR
Yup, thanks.

cc: @carlossanlop
@antonfirsovantonfirsov
mentioned this pull request
Nov 23, 2020
 Merged

scalablecory
scalablecory
approved these changes
Nov 24, 2020
View reviewed changes
@scalablecoryscalablecory
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to improve validation, otherwise lgtm.
Comment threadsrc/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs
@antonfirsov
antonfirsov
commented
Nov 24, 2020

Copy link


Copy Markdown

ContributorAuthor
I've added the docs, can someone double-check the them before we merge?
@geoffkizer
geoffkizer
commented
Nov 24, 2020

Copy link


Copy Markdown

Contributor
LGTM
@antonfirsov
antonfirsov
merged commit 837785f
into
dotnet:masterNov 25, 2020
@ManickaPManickaP
mentioned this pull request
Nov 26, 2020

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 25, 2020
@karelzkarelz
added this to the 6.0.0 milestone
Jan 26, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@halter73halter73halter73 left review comments
@stephentoubstephentoubstephentoub left review comments

+2 more reviewers

@scalablecoryscalablecoryscalablecory approved these changes
@geoffkizergeoffkizergeoffkizer approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Netnew-api-needs-documentation

Projects


None yet


Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

7 participants

@antonfirsov@Dotnet-GitSync-Bot@geoffkizer@stephentoub@halter73@scalablecory@karelz

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Implement TCP Keep-Alive for WinHttpHandler - #44889
Merged
antonfirsov merged 7 commits into
dotnet:masterfrom 
antonfirsov:WinHttp-TcpKeepalive
Nov 25, 2020
Merged
Implement TCP Keep-Alive for WinHttpHandler#44889
antonfirsov merged 7 commits into
dotnet:masterfrom 
antonfirsov:WinHttp-TcpKeepalive
Conversation
@antonfirsov
@antonfirsovantonfirsov
commented
Nov 18, 2020

Copy link


Copy Markdown

Contributor
This implements the API proposal from #44025 (comment) except the  [SupportedOSPlatform("windows10.0.2004")] bits. I prefer to handle that separately in a follow-up PR, since it doesn't seem to be a a functional concern, but rather focuses on managing build targets.
I decided to handle compatibility in a way consistent with other properties:
WinHttpException will be thrown, if OS doesn't support the property.
Contributes to #44025.
/cc @geoffkizer@wfurt@alnikola
@Dotnet-GitSync-Bot
Dotnet-GitSync-Bot
commented
Nov 18, 2020

Copy link


Copy Markdown

Collaborator
Note regarding the new-api-needs-documentation label:
This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.
@ghost
ghost
commented
Nov 18, 2020

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/ncl

See info in area-owners.md if you want to be subscribed.
Details
Issue Details
Description:
This implements the API proposal from #44025 (comment) except the  [SupportedOSPlatform("windows10.0.2004")] bits. I prefer to handle that separately in a follow-up PR, since it doesn't seem to be a a functional concern, but rather focuses on managing build targets.
I decided to handle compatibility in a way consistent with other properties:
WinHttpException will be thrown, if OS doesn't support the property.
Contributes to #44025.
/cc @geoffkizer@wfurt@alnikola
Author:antonfirsov
Assignees:-
Labels:
area-System.Net, new-api-needs-documentation
Milestone:-
geoffkizer
geoffkizer
reviewed
Nov 18, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs
Outdated

 }
 }

 public bool TcpKeepAliveEnabled { get; set; }
@geoffkizergeoffkizerNov 18, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the set here call CheckDisposedOrStarted?
@stephentoubstephentoubNov 18, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default here is false? I'm just questioning that because the comment earlier says "The default settings when a TCP socket is initialized sets the keep-alive timeout to 2 hours and the keep-alive interval to 1 second" which suggests the default here should actually be true?
@antonfirsovantonfirsovNov 19, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default settings when a TCP socket is initialized sets the keep-alive timeout to 2 hours and the keep-alive interval to 1 second
The same docs also state that :
The SO_KEEPALIVE option for a socket is disabled (set to FALSE) by default. [...] When this socket option is enabled, the TCP stack sends keep-alive packets when no data or acknowledgement packets have been received for the connection within an interval.
From this I concluded that if SO_KEEPALIVE  or SIO_KEEPALIVE_VALS are left unchanged, no keepalive packets gonna be sent. We may want to double check with the winsock team since the wording is kinda contradictional.
@stephentoubstephentoubNov 19, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's double-check, and then either update the implementation or the comment :)
@antonfirsovantonfirsovNov 19, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The defaults is false. This discussion resulted in an update of the docs ;)
MicrosoftDocs/win32#635
@halter73halter73Nov 19, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I never see TCP keep-alive packets in HTTP network traces I get from customers, and that would be pretty obvious with the 1 second interval. I know we decided we're sticking with winsock defaults and violating RFC, but the RFC does say TCP keep-alives should default off in no uncertain terms:
 Implementors MAY include "keep-alives" in their TCP
implementations, although this practice is not universally
accepted. If keep-alives are included, the application MUST
be able to turn them on or off for each TCP connection, and
they MUST default to off.

https://tools.ietf.org/html/rfc1122#page-101
I'm glad the winsock defaults at least align with the RFC on this part.
geoffkizer
geoffkizer
reviewed
Nov 18, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http.WinHttpHandler/tests/UnitTests/APICallHistory.cs
Outdated


 public static List<IntPtr> WinHttpOptionClientCertContext { get { return winHttpOptionClientCertContextList; } }

 public static (uint KeepAliveTime, uint KeepAliveInterval)? TcpKeepaliveOptions { get; set; }
@geoffkizergeoffkizerNov 18, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the other properties here are named WinHttpOption*, should we do the same for this one?
geoffkizer
geoffkizer
approved these changes
Nov 18, 2020
View reviewed changes
@geoffkizergeoffkizer
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple small issues above, otherwise LGTM
stephentoub
stephentoub
reviewed
Nov 18, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs
Outdated

 keepaliveinterval = (uint)_tcpKeepAliveInterval.TotalMilliseconds,
 keepalivetime = (uint)_tcpKeepAliveTime.TotalMilliseconds
 };
 void* ptr = &tcpKeepalive;
@stephentoubstephentoubNov 18, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this local seems unnecessary, and you can just use (IntPtr)&tcpKeepalive below.
@runfoapprunfoappBot
mentioned this pull request
Nov 18, 2020

Closed

@antonfirsov

This comment has been minimized.

Sign in to view
@azure-pipelines

This comment has been minimized.

Sign in to view
@antonfirsov
antonfirsov
commented
Nov 19, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@geoffkizer I think all finding have been addressed, wondering if it's worth to wait for a few more approvals for this.
@geoffkizer
geoffkizer
commented
Nov 19, 2020

Copy link


Copy Markdown

Contributor
LGTM
@antonfirsov
antonfirsov
commented
Nov 23, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@geoffkizer@stephentoub what about documentation? Should I add xmldoc right in the PR, or is it sufficient to have the new-api-needs-documentation label? Do we apply the same rules for NuGet libraries as for the rest?
@stephentoub
stephentoub
commented
Nov 23, 2020

Copy link


Copy Markdown

Member
Should I add xmldoc right in the PR
Yup, thanks.

cc: @carlossanlop
@antonfirsovantonfirsov
mentioned this pull request
Nov 23, 2020
 Merged

scalablecory
scalablecory
approved these changes
Nov 24, 2020
View reviewed changes
@scalablecoryscalablecory
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to improve validation, otherwise lgtm.
Comment threadsrc/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs
@antonfirsov
antonfirsov
commented
Nov 24, 2020

Copy link


Copy Markdown

ContributorAuthor
I've added the docs, can someone double-check the them before we merge?
@geoffkizer
geoffkizer
commented
Nov 24, 2020

Copy link


Copy Markdown

Contributor
LGTM
@antonfirsov
antonfirsov
merged commit 837785f
into
dotnet:masterNov 25, 2020
@ManickaPManickaP
mentioned this pull request
Nov 26, 2020

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 25, 2020
@karelzkarelz
added this to the 6.0.0 milestone
Jan 26, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@halter73halter73halter73 left review comments
@stephentoubstephentoubstephentoub left review comments

+2 more reviewers

@scalablecoryscalablecoryscalablecory approved these changes
@geoffkizergeoffkizergeoffkizer approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Netnew-api-needs-documentation

Projects


None yet


Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

7 participants

@antonfirsov@Dotnet-GitSync-Bot@geoffkizer@stephentoub@halter73@scalablecory@karelz

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Implement TCP Keep-Alive for WinHttpHandler - #44889
Merged
antonfirsov merged 7 commits into
dotnet:masterfrom 
antonfirsov:WinHttp-TcpKeepalive
Nov 25, 2020
Merged
Implement TCP Keep-Alive for WinHttpHandler#44889
antonfirsov merged 7 commits into
dotnet:masterfrom 
antonfirsov:WinHttp-TcpKeepalive
Conversation
@antonfirsov
@antonfirsovantonfirsov
commented
Nov 18, 2020

Copy link


Copy Markdown

Contributor
This implements the API proposal from #44025 (comment) except the  [SupportedOSPlatform("windows10.0.2004")] bits. I prefer to handle that separately in a follow-up PR, since it doesn't seem to be a a functional concern, but rather focuses on managing build targets.
I decided to handle compatibility in a way consistent with other properties:
WinHttpException will be thrown, if OS doesn't support the property.
Contributes to #44025.
/cc @geoffkizer@wfurt@alnikola
@Dotnet-GitSync-Bot
Dotnet-GitSync-Bot
commented
Nov 18, 2020

Copy link


Copy Markdown

Collaborator
Note regarding the new-api-needs-documentation label:
This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.
@ghost
ghost
commented
Nov 18, 2020

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/ncl

See info in area-owners.md if you want to be subscribed.
Details
Issue Details
Description:
This implements the API proposal from #44025 (comment) except the  [SupportedOSPlatform("windows10.0.2004")] bits. I prefer to handle that separately in a follow-up PR, since it doesn't seem to be a a functional concern, but rather focuses on managing build targets.
I decided to handle compatibility in a way consistent with other properties:
WinHttpException will be thrown, if OS doesn't support the property.
Contributes to #44025.
/cc @geoffkizer@wfurt@alnikola
Author:antonfirsov
Assignees:-
Labels:
area-System.Net, new-api-needs-documentation
Milestone:-
geoffkizer
geoffkizer
reviewed
Nov 18, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs
Outdated

 }
 }

 public bool TcpKeepAliveEnabled { get; set; }
@geoffkizergeoffkizerNov 18, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the set here call CheckDisposedOrStarted?
@stephentoubstephentoubNov 18, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default here is false? I'm just questioning that because the comment earlier says "The default settings when a TCP socket is initialized sets the keep-alive timeout to 2 hours and the keep-alive interval to 1 second" which suggests the default here should actually be true?
@antonfirsovantonfirsovNov 19, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default settings when a TCP socket is initialized sets the keep-alive timeout to 2 hours and the keep-alive interval to 1 second
The same docs also state that :
The SO_KEEPALIVE option for a socket is disabled (set to FALSE) by default. [...] When this socket option is enabled, the TCP stack sends keep-alive packets when no data or acknowledgement packets have been received for the connection within an interval.
From this I concluded that if SO_KEEPALIVE  or SIO_KEEPALIVE_VALS are left unchanged, no keepalive packets gonna be sent. We may want to double check with the winsock team since the wording is kinda contradictional.
@stephentoubstephentoubNov 19, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's double-check, and then either update the implementation or the comment :)
@antonfirsovantonfirsovNov 19, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The defaults is false. This discussion resulted in an update of the docs ;)
MicrosoftDocs/win32#635
@halter73halter73Nov 19, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I never see TCP keep-alive packets in HTTP network traces I get from customers, and that would be pretty obvious with the 1 second interval. I know we decided we're sticking with winsock defaults and violating RFC, but the RFC does say TCP keep-alives should default off in no uncertain terms:
 Implementors MAY include "keep-alives" in their TCP
implementations, although this practice is not universally
accepted. If keep-alives are included, the application MUST
be able to turn them on or off for each TCP connection, and
they MUST default to off.

https://tools.ietf.org/html/rfc1122#page-101
I'm glad the winsock defaults at least align with the RFC on this part.
geoffkizer
geoffkizer
reviewed
Nov 18, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http.WinHttpHandler/tests/UnitTests/APICallHistory.cs
Outdated


 public static List<IntPtr> WinHttpOptionClientCertContext { get { return winHttpOptionClientCertContextList; } }

 public static (uint KeepAliveTime, uint KeepAliveInterval)? TcpKeepaliveOptions { get; set; }
@geoffkizergeoffkizerNov 18, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the other properties here are named WinHttpOption*, should we do the same for this one?
geoffkizer
geoffkizer
approved these changes
Nov 18, 2020
View reviewed changes
@geoffkizergeoffkizer
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple small issues above, otherwise LGTM
stephentoub
stephentoub
reviewed
Nov 18, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs
Outdated

 keepaliveinterval = (uint)_tcpKeepAliveInterval.TotalMilliseconds,
 keepalivetime = (uint)_tcpKeepAliveTime.TotalMilliseconds
 };
 void* ptr = &tcpKeepalive;
@stephentoubstephentoubNov 18, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this local seems unnecessary, and you can just use (IntPtr)&tcpKeepalive below.
@runfoapprunfoappBot
mentioned this pull request
Nov 18, 2020

Closed

@antonfirsov

This comment has been minimized.

Sign in to view
@azure-pipelines

This comment has been minimized.

Sign in to view
@antonfirsov
antonfirsov
commented
Nov 19, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@geoffkizer I think all finding have been addressed, wondering if it's worth to wait for a few more approvals for this.
@geoffkizer
geoffkizer
commented
Nov 19, 2020

Copy link


Copy Markdown

Contributor
LGTM
@antonfirsov
antonfirsov
commented
Nov 23, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@geoffkizer@stephentoub what about documentation? Should I add xmldoc right in the PR, or is it sufficient to have the new-api-needs-documentation label? Do we apply the same rules for NuGet libraries as for the rest?
@stephentoub
stephentoub
commented
Nov 23, 2020

Copy link


Copy Markdown

Member
Should I add xmldoc right in the PR
Yup, thanks.

cc: @carlossanlop
@antonfirsovantonfirsov
mentioned this pull request
Nov 23, 2020
 Merged

scalablecory
scalablecory
approved these changes
Nov 24, 2020
View reviewed changes
@scalablecoryscalablecory
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to improve validation, otherwise lgtm.
Comment threadsrc/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs
@antonfirsov
antonfirsov
commented
Nov 24, 2020

Copy link


Copy Markdown

ContributorAuthor
I've added the docs, can someone double-check the them before we merge?
@geoffkizer
geoffkizer
commented
Nov 24, 2020

Copy link


Copy Markdown

Contributor
LGTM
@antonfirsov
antonfirsov
merged commit 837785f
into
dotnet:masterNov 25, 2020
@ManickaPManickaP
mentioned this pull request
Nov 26, 2020

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 25, 2020
@karelzkarelz
added this to the 6.0.0 milestone
Jan 26, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@halter73halter73halter73 left review comments
@stephentoubstephentoubstephentoub left review comments

+2 more reviewers

@scalablecoryscalablecoryscalablecory approved these changes
@geoffkizergeoffkizergeoffkizer approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Netnew-api-needs-documentation

Projects


None yet


Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

7 participants

@antonfirsov@Dotnet-GitSync-Bot@geoffkizer@stephentoub@halter73@scalablecory@karelz

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Implement TCP Keep-Alive for WinHttpHandler - #44889
Merged
antonfirsov merged 7 commits into
dotnet:masterfrom 
antonfirsov:WinHttp-TcpKeepalive
Nov 25, 2020
Merged
Implement TCP Keep-Alive for WinHttpHandler#44889
antonfirsov merged 7 commits into
dotnet:masterfrom 
antonfirsov:WinHttp-TcpKeepalive
Conversation
@antonfirsov
@antonfirsovantonfirsov
commented
Nov 18, 2020

Copy link


Copy Markdown

Contributor
This implements the API proposal from #44025 (comment) except the  [SupportedOSPlatform("windows10.0.2004")] bits. I prefer to handle that separately in a follow-up PR, since it doesn't seem to be a a functional concern, but rather focuses on managing build targets.
I decided to handle compatibility in a way consistent with other properties:
WinHttpException will be thrown, if OS doesn't support the property.
Contributes to #44025.
/cc @geoffkizer@wfurt@alnikola
@Dotnet-GitSync-Bot
Dotnet-GitSync-Bot
commented
Nov 18, 2020

Copy link


Copy Markdown

Collaborator
Note regarding the new-api-needs-documentation label:
This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.
@ghost
ghost
commented
Nov 18, 2020

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/ncl

See info in area-owners.md if you want to be subscribed.
Details
Issue Details
Description:
This implements the API proposal from #44025 (comment) except the  [SupportedOSPlatform("windows10.0.2004")] bits. I prefer to handle that separately in a follow-up PR, since it doesn't seem to be a a functional concern, but rather focuses on managing build targets.
I decided to handle compatibility in a way consistent with other properties:
WinHttpException will be thrown, if OS doesn't support the property.
Contributes to #44025.
/cc @geoffkizer@wfurt@alnikola
Author:antonfirsov
Assignees:-
Labels:
area-System.Net, new-api-needs-documentation
Milestone:-
geoffkizer
geoffkizer
reviewed
Nov 18, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs
Outdated

 }
 }

 public bool TcpKeepAliveEnabled { get; set; }
@geoffkizergeoffkizerNov 18, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the set here call CheckDisposedOrStarted?
@stephentoubstephentoubNov 18, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default here is false? I'm just questioning that because the comment earlier says "The default settings when a TCP socket is initialized sets the keep-alive timeout to 2 hours and the keep-alive interval to 1 second" which suggests the default here should actually be true?
@antonfirsovantonfirsovNov 19, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default settings when a TCP socket is initialized sets the keep-alive timeout to 2 hours and the keep-alive interval to 1 second
The same docs also state that :
The SO_KEEPALIVE option for a socket is disabled (set to FALSE) by default. [...] When this socket option is enabled, the TCP stack sends keep-alive packets when no data or acknowledgement packets have been received for the connection within an interval.
From this I concluded that if SO_KEEPALIVE  or SIO_KEEPALIVE_VALS are left unchanged, no keepalive packets gonna be sent. We may want to double check with the winsock team since the wording is kinda contradictional.
@stephentoubstephentoubNov 19, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's double-check, and then either update the implementation or the comment :)
@antonfirsovantonfirsovNov 19, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The defaults is false. This discussion resulted in an update of the docs ;)
MicrosoftDocs/win32#635
@halter73halter73Nov 19, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I never see TCP keep-alive packets in HTTP network traces I get from customers, and that would be pretty obvious with the 1 second interval. I know we decided we're sticking with winsock defaults and violating RFC, but the RFC does say TCP keep-alives should default off in no uncertain terms:
 Implementors MAY include "keep-alives" in their TCP
implementations, although this practice is not universally
accepted. If keep-alives are included, the application MUST
be able to turn them on or off for each TCP connection, and
they MUST default to off.

https://tools.ietf.org/html/rfc1122#page-101
I'm glad the winsock defaults at least align with the RFC on this part.
geoffkizer
geoffkizer
reviewed
Nov 18, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http.WinHttpHandler/tests/UnitTests/APICallHistory.cs
Outdated


 public static List<IntPtr> WinHttpOptionClientCertContext { get { return winHttpOptionClientCertContextList; } }

 public static (uint KeepAliveTime, uint KeepAliveInterval)? TcpKeepaliveOptions { get; set; }
@geoffkizergeoffkizerNov 18, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the other properties here are named WinHttpOption*, should we do the same for this one?
geoffkizer
geoffkizer
approved these changes
Nov 18, 2020
View reviewed changes
@geoffkizergeoffkizer
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple small issues above, otherwise LGTM
stephentoub
stephentoub
reviewed
Nov 18, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs
Outdated

 keepaliveinterval = (uint)_tcpKeepAliveInterval.TotalMilliseconds,
 keepalivetime = (uint)_tcpKeepAliveTime.TotalMilliseconds
 };
 void* ptr = &tcpKeepalive;
@stephentoubstephentoubNov 18, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this local seems unnecessary, and you can just use (IntPtr)&tcpKeepalive below.
@runfoapprunfoappBot
mentioned this pull request
Nov 18, 2020

Closed

@antonfirsov

This comment has been minimized.

Sign in to view
@azure-pipelines

This comment has been minimized.

Sign in to view
@antonfirsov
antonfirsov
commented
Nov 19, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@geoffkizer I think all finding have been addressed, wondering if it's worth to wait for a few more approvals for this.
@geoffkizer
geoffkizer
commented
Nov 19, 2020

Copy link


Copy Markdown

Contributor
LGTM
@antonfirsov
antonfirsov
commented
Nov 23, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@geoffkizer@stephentoub what about documentation? Should I add xmldoc right in the PR, or is it sufficient to have the new-api-needs-documentation label? Do we apply the same rules for NuGet libraries as for the rest?
@stephentoub
stephentoub
commented
Nov 23, 2020

Copy link


Copy Markdown

Member
Should I add xmldoc right in the PR
Yup, thanks.

cc: @carlossanlop
@antonfirsovantonfirsov
mentioned this pull request
Nov 23, 2020
 Merged

scalablecory
scalablecory
approved these changes
Nov 24, 2020
View reviewed changes
@scalablecoryscalablecory
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to improve validation, otherwise lgtm.
Comment threadsrc/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs
@antonfirsov
antonfirsov
commented
Nov 24, 2020

Copy link


Copy Markdown

ContributorAuthor
I've added the docs, can someone double-check the them before we merge?
@geoffkizer
geoffkizer
commented
Nov 24, 2020

Copy link


Copy Markdown

Contributor
LGTM
@antonfirsov
antonfirsov
merged commit 837785f
into
dotnet:masterNov 25, 2020
@ManickaPManickaP
mentioned this pull request
Nov 26, 2020

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 25, 2020
@karelzkarelz
added this to the 6.0.0 milestone
Jan 26, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@halter73halter73halter73 left review comments
@stephentoubstephentoubstephentoub left review comments

+2 more reviewers

@scalablecoryscalablecoryscalablecory approved these changes
@geoffkizergeoffkizergeoffkizer approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Netnew-api-needs-documentation

Projects


None yet


Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

7 participants

@antonfirsov@Dotnet-GitSync-Bot@geoffkizer@stephentoub@halter73@scalablecory@karelz

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Implement TCP Keep-Alive for WinHttpHandler - #44889
Merged
antonfirsov merged 7 commits into
dotnet:masterfrom 
antonfirsov:WinHttp-TcpKeepalive
Nov 25, 2020
Merged
Implement TCP Keep-Alive for WinHttpHandler#44889
antonfirsov merged 7 commits into
dotnet:masterfrom 
antonfirsov:WinHttp-TcpKeepalive
Conversation
@antonfirsov
@antonfirsovantonfirsov
commented
Nov 18, 2020

Copy link


Copy Markdown

Contributor
This implements the API proposal from #44025 (comment) except the  [SupportedOSPlatform("windows10.0.2004")] bits. I prefer to handle that separately in a follow-up PR, since it doesn't seem to be a a functional concern, but rather focuses on managing build targets.
I decided to handle compatibility in a way consistent with other properties:
WinHttpException will be thrown, if OS doesn't support the property.
Contributes to #44025.
/cc @geoffkizer@wfurt@alnikola
@Dotnet-GitSync-Bot
Dotnet-GitSync-Bot
commented
Nov 18, 2020

Copy link


Copy Markdown

Collaborator
Note regarding the new-api-needs-documentation label:
This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.
@ghost
ghost
commented
Nov 18, 2020

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/ncl

See info in area-owners.md if you want to be subscribed.
Details
Issue Details
Description:
This implements the API proposal from #44025 (comment) except the  [SupportedOSPlatform("windows10.0.2004")] bits. I prefer to handle that separately in a follow-up PR, since it doesn't seem to be a a functional concern, but rather focuses on managing build targets.
I decided to handle compatibility in a way consistent with other properties:
WinHttpException will be thrown, if OS doesn't support the property.
Contributes to #44025.
/cc @geoffkizer@wfurt@alnikola
Author:antonfirsov
Assignees:-
Labels:
area-System.Net, new-api-needs-documentation
Milestone:-
geoffkizer
geoffkizer
reviewed
Nov 18, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs
Outdated

 }
 }

 public bool TcpKeepAliveEnabled { get; set; }
@geoffkizergeoffkizerNov 18, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the set here call CheckDisposedOrStarted?
@stephentoubstephentoubNov 18, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default here is false? I'm just questioning that because the comment earlier says "The default settings when a TCP socket is initialized sets the keep-alive timeout to 2 hours and the keep-alive interval to 1 second" which suggests the default here should actually be true?
@antonfirsovantonfirsovNov 19, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default settings when a TCP socket is initialized sets the keep-alive timeout to 2 hours and the keep-alive interval to 1 second
The same docs also state that :
The SO_KEEPALIVE option for a socket is disabled (set to FALSE) by default. [...] When this socket option is enabled, the TCP stack sends keep-alive packets when no data or acknowledgement packets have been received for the connection within an interval.
From this I concluded that if SO_KEEPALIVE  or SIO_KEEPALIVE_VALS are left unchanged, no keepalive packets gonna be sent. We may want to double check with the winsock team since the wording is kinda contradictional.
@stephentoubstephentoubNov 19, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's double-check, and then either update the implementation or the comment :)
@antonfirsovantonfirsovNov 19, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The defaults is false. This discussion resulted in an update of the docs ;)
MicrosoftDocs/win32#635
@halter73halter73Nov 19, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I never see TCP keep-alive packets in HTTP network traces I get from customers, and that would be pretty obvious with the 1 second interval. I know we decided we're sticking with winsock defaults and violating RFC, but the RFC does say TCP keep-alives should default off in no uncertain terms:
 Implementors MAY include "keep-alives" in their TCP
implementations, although this practice is not universally
accepted. If keep-alives are included, the application MUST
be able to turn them on or off for each TCP connection, and
they MUST default to off.

https://tools.ietf.org/html/rfc1122#page-101
I'm glad the winsock defaults at least align with the RFC on this part.
geoffkizer
geoffkizer
reviewed
Nov 18, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http.WinHttpHandler/tests/UnitTests/APICallHistory.cs
Outdated


 public static List<IntPtr> WinHttpOptionClientCertContext { get { return winHttpOptionClientCertContextList; } }

 public static (uint KeepAliveTime, uint KeepAliveInterval)? TcpKeepaliveOptions { get; set; }
@geoffkizergeoffkizerNov 18, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the other properties here are named WinHttpOption*, should we do the same for this one?
geoffkizer
geoffkizer
approved these changes
Nov 18, 2020
View reviewed changes
@geoffkizergeoffkizer
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple small issues above, otherwise LGTM
stephentoub
stephentoub
reviewed
Nov 18, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs
Outdated

 keepaliveinterval = (uint)_tcpKeepAliveInterval.TotalMilliseconds,
 keepalivetime = (uint)_tcpKeepAliveTime.TotalMilliseconds
 };
 void* ptr = &tcpKeepalive;
@stephentoubstephentoubNov 18, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this local seems unnecessary, and you can just use (IntPtr)&tcpKeepalive below.
@runfoapprunfoappBot
mentioned this pull request
Nov 18, 2020

Closed

@antonfirsov

This comment has been minimized.

Sign in to view
@azure-pipelines

This comment has been minimized.

Sign in to view
@antonfirsov
antonfirsov
commented
Nov 19, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@geoffkizer I think all finding have been addressed, wondering if it's worth to wait for a few more approvals for this.
@geoffkizer
geoffkizer
commented
Nov 19, 2020

Copy link


Copy Markdown

Contributor
LGTM
@antonfirsov
antonfirsov
commented
Nov 23, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@geoffkizer@stephentoub what about documentation? Should I add xmldoc right in the PR, or is it sufficient to have the new-api-needs-documentation label? Do we apply the same rules for NuGet libraries as for the rest?
@stephentoub
stephentoub
commented
Nov 23, 2020

Copy link


Copy Markdown

Member
Should I add xmldoc right in the PR
Yup, thanks.

cc: @carlossanlop
@antonfirsovantonfirsov
mentioned this pull request
Nov 23, 2020
 Merged

scalablecory
scalablecory
approved these changes
Nov 24, 2020
View reviewed changes
@scalablecoryscalablecory
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to improve validation, otherwise lgtm.
Comment threadsrc/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs
@antonfirsov
antonfirsov
commented
Nov 24, 2020

Copy link


Copy Markdown

ContributorAuthor
I've added the docs, can someone double-check the them before we merge?
@geoffkizer
geoffkizer
commented
Nov 24, 2020

Copy link


Copy Markdown

Contributor
LGTM
@antonfirsov
antonfirsov
merged commit 837785f
into
dotnet:masterNov 25, 2020
@ManickaPManickaP
mentioned this pull request
Nov 26, 2020

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 25, 2020
@karelzkarelz
added this to the 6.0.0 milestone
Jan 26, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@halter73halter73halter73 left review comments
@stephentoubstephentoubstephentoub left review comments

+2 more reviewers

@scalablecoryscalablecoryscalablecory approved these changes
@geoffkizergeoffkizergeoffkizer approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Netnew-api-needs-documentation

Projects


None yet


Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

7 participants

@antonfirsov@Dotnet-GitSync-Bot@geoffkizer@stephentoub@halter73@scalablecory@karelz

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Implement TCP Keep-Alive for WinHttpHandler - #44889
Merged
antonfirsov merged 7 commits into
dotnet:masterfrom 
antonfirsov:WinHttp-TcpKeepalive
Nov 25, 2020
Merged
Implement TCP Keep-Alive for WinHttpHandler#44889
antonfirsov merged 7 commits into
dotnet:masterfrom 
antonfirsov:WinHttp-TcpKeepalive
Conversation
@antonfirsov
@antonfirsovantonfirsov
commented
Nov 18, 2020

Copy link


Copy Markdown

Contributor
This implements the API proposal from #44025 (comment) except the  [SupportedOSPlatform("windows10.0.2004")] bits. I prefer to handle that separately in a follow-up PR, since it doesn't seem to be a a functional concern, but rather focuses on managing build targets.
I decided to handle compatibility in a way consistent with other properties:
WinHttpException will be thrown, if OS doesn't support the property.
Contributes to #44025.
/cc @geoffkizer@wfurt@alnikola
@Dotnet-GitSync-Bot
Dotnet-GitSync-Bot
commented
Nov 18, 2020

Copy link


Copy Markdown

Collaborator
Note regarding the new-api-needs-documentation label:
This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.
@ghost
ghost
commented
Nov 18, 2020

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/ncl

See info in area-owners.md if you want to be subscribed.
Details
Issue Details
Description:
This implements the API proposal from #44025 (comment) except the  [SupportedOSPlatform("windows10.0.2004")] bits. I prefer to handle that separately in a follow-up PR, since it doesn't seem to be a a functional concern, but rather focuses on managing build targets.
I decided to handle compatibility in a way consistent with other properties:
WinHttpException will be thrown, if OS doesn't support the property.
Contributes to #44025.
/cc @geoffkizer@wfurt@alnikola
Author:antonfirsov
Assignees:-
Labels:
area-System.Net, new-api-needs-documentation
Milestone:-
geoffkizer
geoffkizer
reviewed
Nov 18, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs
Outdated

 }
 }

 public bool TcpKeepAliveEnabled { get; set; }
@geoffkizergeoffkizerNov 18, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the set here call CheckDisposedOrStarted?
@stephentoubstephentoubNov 18, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default here is false? I'm just questioning that because the comment earlier says "The default settings when a TCP socket is initialized sets the keep-alive timeout to 2 hours and the keep-alive interval to 1 second" which suggests the default here should actually be true?
@antonfirsovantonfirsovNov 19, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default settings when a TCP socket is initialized sets the keep-alive timeout to 2 hours and the keep-alive interval to 1 second
The same docs also state that :
The SO_KEEPALIVE option for a socket is disabled (set to FALSE) by default. [...] When this socket option is enabled, the TCP stack sends keep-alive packets when no data or acknowledgement packets have been received for the connection within an interval.
From this I concluded that if SO_KEEPALIVE  or SIO_KEEPALIVE_VALS are left unchanged, no keepalive packets gonna be sent. We may want to double check with the winsock team since the wording is kinda contradictional.
@stephentoubstephentoubNov 19, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's double-check, and then either update the implementation or the comment :)
@antonfirsovantonfirsovNov 19, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The defaults is false. This discussion resulted in an update of the docs ;)
MicrosoftDocs/win32#635
@halter73halter73Nov 19, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I never see TCP keep-alive packets in HTTP network traces I get from customers, and that would be pretty obvious with the 1 second interval. I know we decided we're sticking with winsock defaults and violating RFC, but the RFC does say TCP keep-alives should default off in no uncertain terms:
 Implementors MAY include "keep-alives" in their TCP
implementations, although this practice is not universally
accepted. If keep-alives are included, the application MUST
be able to turn them on or off for each TCP connection, and
they MUST default to off.

https://tools.ietf.org/html/rfc1122#page-101
I'm glad the winsock defaults at least align with the RFC on this part.
geoffkizer
geoffkizer
reviewed
Nov 18, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http.WinHttpHandler/tests/UnitTests/APICallHistory.cs
Outdated


 public static List<IntPtr> WinHttpOptionClientCertContext { get { return winHttpOptionClientCertContextList; } }

 public static (uint KeepAliveTime, uint KeepAliveInterval)? TcpKeepaliveOptions { get; set; }
@geoffkizergeoffkizerNov 18, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the other properties here are named WinHttpOption*, should we do the same for this one?
geoffkizer
geoffkizer
approved these changes
Nov 18, 2020
View reviewed changes
@geoffkizergeoffkizer
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple small issues above, otherwise LGTM
stephentoub
stephentoub
reviewed
Nov 18, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs
Outdated

 keepaliveinterval = (uint)_tcpKeepAliveInterval.TotalMilliseconds,
 keepalivetime = (uint)_tcpKeepAliveTime.TotalMilliseconds
 };
 void* ptr = &tcpKeepalive;
@stephentoubstephentoubNov 18, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this local seems unnecessary, and you can just use (IntPtr)&tcpKeepalive below.
@runfoapprunfoappBot
mentioned this pull request
Nov 18, 2020

Closed

@antonfirsov

This comment has been minimized.

Sign in to view
@azure-pipelines

This comment has been minimized.

Sign in to view
@antonfirsov
antonfirsov
commented
Nov 19, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@geoffkizer I think all finding have been addressed, wondering if it's worth to wait for a few more approvals for this.
@geoffkizer
geoffkizer
commented
Nov 19, 2020

Copy link


Copy Markdown

Contributor
LGTM
@antonfirsov
antonfirsov
commented
Nov 23, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@geoffkizer@stephentoub what about documentation? Should I add xmldoc right in the PR, or is it sufficient to have the new-api-needs-documentation label? Do we apply the same rules for NuGet libraries as for the rest?
@stephentoub
stephentoub
commented
Nov 23, 2020

Copy link


Copy Markdown

Member
Should I add xmldoc right in the PR
Yup, thanks.

cc: @carlossanlop
@antonfirsovantonfirsov
mentioned this pull request
Nov 23, 2020
 Merged

scalablecory
scalablecory
approved these changes
Nov 24, 2020
View reviewed changes
@scalablecoryscalablecory
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to improve validation, otherwise lgtm.
Comment threadsrc/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs
@antonfirsov
antonfirsov
commented
Nov 24, 2020

Copy link


Copy Markdown

ContributorAuthor
I've added the docs, can someone double-check the them before we merge?
@geoffkizer
geoffkizer
commented
Nov 24, 2020

Copy link


Copy Markdown

Contributor
LGTM
@antonfirsov
antonfirsov
merged commit 837785f
into
dotnet:masterNov 25, 2020
@ManickaPManickaP
mentioned this pull request
Nov 26, 2020

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 25, 2020
@karelzkarelz
added this to the 6.0.0 milestone
Jan 26, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@halter73halter73halter73 left review comments
@stephentoubstephentoubstephentoub left review comments

+2 more reviewers

@scalablecoryscalablecoryscalablecory approved these changes
@geoffkizergeoffkizergeoffkizer approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Netnew-api-needs-documentation

Projects


None yet


Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

7 participants

@antonfirsov@Dotnet-GitSync-Bot@geoffkizer@stephentoub@halter73@scalablecory@karelz

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Implement TCP Keep-Alive for WinHttpHandler - #44889
Merged
antonfirsov merged 7 commits into
dotnet:masterfrom 
antonfirsov:WinHttp-TcpKeepalive
Nov 25, 2020
Merged
Implement TCP Keep-Alive for WinHttpHandler#44889
antonfirsov merged 7 commits into
dotnet:masterfrom 
antonfirsov:WinHttp-TcpKeepalive
Conversation
@antonfirsov
@antonfirsovantonfirsov
commented
Nov 18, 2020

Copy link


Copy Markdown

Contributor
This implements the API proposal from #44025 (comment) except the  [SupportedOSPlatform("windows10.0.2004")] bits. I prefer to handle that separately in a follow-up PR, since it doesn't seem to be a a functional concern, but rather focuses on managing build targets.
I decided to handle compatibility in a way consistent with other properties:
WinHttpException will be thrown, if OS doesn't support the property.
Contributes to #44025.
/cc @geoffkizer@wfurt@alnikola
@Dotnet-GitSync-Bot
Dotnet-GitSync-Bot
commented
Nov 18, 2020

Copy link


Copy Markdown

Collaborator
Note regarding the new-api-needs-documentation label:
This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.
@ghost
ghost
commented
Nov 18, 2020

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/ncl

See info in area-owners.md if you want to be subscribed.
Details
Issue Details
Description:
This implements the API proposal from #44025 (comment) except the  [SupportedOSPlatform("windows10.0.2004")] bits. I prefer to handle that separately in a follow-up PR, since it doesn't seem to be a a functional concern, but rather focuses on managing build targets.
I decided to handle compatibility in a way consistent with other properties:
WinHttpException will be thrown, if OS doesn't support the property.
Contributes to #44025.
/cc @geoffkizer@wfurt@alnikola
Author:antonfirsov
Assignees:-
Labels:
area-System.Net, new-api-needs-documentation
Milestone:-
geoffkizer
geoffkizer
reviewed
Nov 18, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs
Outdated

 }
 }

 public bool TcpKeepAliveEnabled { get; set; }
@geoffkizergeoffkizerNov 18, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the set here call CheckDisposedOrStarted?
@stephentoubstephentoubNov 18, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default here is false? I'm just questioning that because the comment earlier says "The default settings when a TCP socket is initialized sets the keep-alive timeout to 2 hours and the keep-alive interval to 1 second" which suggests the default here should actually be true?
@antonfirsovantonfirsovNov 19, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default settings when a TCP socket is initialized sets the keep-alive timeout to 2 hours and the keep-alive interval to 1 second
The same docs also state that :
The SO_KEEPALIVE option for a socket is disabled (set to FALSE) by default. [...] When this socket option is enabled, the TCP stack sends keep-alive packets when no data or acknowledgement packets have been received for the connection within an interval.
From this I concluded that if SO_KEEPALIVE  or SIO_KEEPALIVE_VALS are left unchanged, no keepalive packets gonna be sent. We may want to double check with the winsock team since the wording is kinda contradictional.
@stephentoubstephentoubNov 19, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's double-check, and then either update the implementation or the comment :)
@antonfirsovantonfirsovNov 19, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The defaults is false. This discussion resulted in an update of the docs ;)
MicrosoftDocs/win32#635
@halter73halter73Nov 19, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I never see TCP keep-alive packets in HTTP network traces I get from customers, and that would be pretty obvious with the 1 second interval. I know we decided we're sticking with winsock defaults and violating RFC, but the RFC does say TCP keep-alives should default off in no uncertain terms:
 Implementors MAY include "keep-alives" in their TCP
implementations, although this practice is not universally
accepted. If keep-alives are included, the application MUST
be able to turn them on or off for each TCP connection, and
they MUST default to off.

https://tools.ietf.org/html/rfc1122#page-101
I'm glad the winsock defaults at least align with the RFC on this part.
geoffkizer
geoffkizer
reviewed
Nov 18, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http.WinHttpHandler/tests/UnitTests/APICallHistory.cs
Outdated


 public static List<IntPtr> WinHttpOptionClientCertContext { get { return winHttpOptionClientCertContextList; } }

 public static (uint KeepAliveTime, uint KeepAliveInterval)? TcpKeepaliveOptions { get; set; }
@geoffkizergeoffkizerNov 18, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the other properties here are named WinHttpOption*, should we do the same for this one?
geoffkizer
geoffkizer
approved these changes
Nov 18, 2020
View reviewed changes
@geoffkizergeoffkizer
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple small issues above, otherwise LGTM
stephentoub
stephentoub
reviewed
Nov 18, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs
Outdated

 keepaliveinterval = (uint)_tcpKeepAliveInterval.TotalMilliseconds,
 keepalivetime = (uint)_tcpKeepAliveTime.TotalMilliseconds
 };
 void* ptr = &tcpKeepalive;
@stephentoubstephentoubNov 18, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this local seems unnecessary, and you can just use (IntPtr)&tcpKeepalive below.
@runfoapprunfoappBot
mentioned this pull request
Nov 18, 2020

Closed

@antonfirsov

This comment has been minimized.

Sign in to view
@azure-pipelines

This comment has been minimized.

Sign in to view
@antonfirsov
antonfirsov
commented
Nov 19, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@geoffkizer I think all finding have been addressed, wondering if it's worth to wait for a few more approvals for this.
@geoffkizer
geoffkizer
commented
Nov 19, 2020

Copy link


Copy Markdown

Contributor
LGTM
@antonfirsov
antonfirsov
commented
Nov 23, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@geoffkizer@stephentoub what about documentation? Should I add xmldoc right in the PR, or is it sufficient to have the new-api-needs-documentation label? Do we apply the same rules for NuGet libraries as for the rest?
@stephentoub
stephentoub
commented
Nov 23, 2020

Copy link


Copy Markdown

Member
Should I add xmldoc right in the PR
Yup, thanks.

cc: @carlossanlop
@antonfirsovantonfirsov
mentioned this pull request
Nov 23, 2020
 Merged

scalablecory
scalablecory
approved these changes
Nov 24, 2020
View reviewed changes
@scalablecoryscalablecory
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to improve validation, otherwise lgtm.
Comment threadsrc/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs
@antonfirsov
antonfirsov
commented
Nov 24, 2020

Copy link


Copy Markdown

ContributorAuthor
I've added the docs, can someone double-check the them before we merge?
@geoffkizer
geoffkizer
commented
Nov 24, 2020

Copy link


Copy Markdown

Contributor
LGTM
@antonfirsov
antonfirsov
merged commit 837785f
into
dotnet:masterNov 25, 2020
@ManickaPManickaP
mentioned this pull request
Nov 26, 2020

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 25, 2020
@karelzkarelz
added this to the 6.0.0 milestone
Jan 26, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@halter73halter73halter73 left review comments
@stephentoubstephentoubstephentoub left review comments

+2 more reviewers

@scalablecoryscalablecoryscalablecory approved these changes
@geoffkizergeoffkizergeoffkizer approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Netnew-api-needs-documentation

Projects


None yet


Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

7 participants

@antonfirsov@Dotnet-GitSync-Bot@geoffkizer@stephentoub@halter73@scalablecory@karelz