Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change Linux fetch timeout behavior for chain building #40676

Merged
merged 8 commits into from
Aug 13, 2020

Conversation

vcsjones
Copy link
Member

@vcsjones vcsjones commented Aug 11, 2020

This gets the ball rolling on what I guess is going to require some discussion to unify UrlRetreivalTimeout on Windows / Linux.

Change the UrlRetrievalTimeout behavior on Linux to match Windows' existing behavior of using a per-operation timeout rather than cumulative.

Add some tests for operations that time out.

Contributes to #38875
Closes #40578

Change the UrlRetrievalTimeout behavior on Linux to match Windows'
existing behavior of using a per-operation timeout rather than
cumulative.

Add some tests for operations that time out.
@ghost
Copy link

ghost commented Aug 11, 2020

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq
See info in area-owners.md if you want to be subscribed.

@vcsjones vcsjones changed the title Change Linux fetch timeout behavior. Change Linux fetch timeout behavior for chain building Aug 11, 2020
@@ -30,6 +30,9 @@ internal sealed class RevocationResponder : IDisposable

public bool RespondEmpty { get; set; }

public TimeSpan RespondDelay { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public TimeSpan RespondDelay { get; set; }
public TimeSpan ResponseDelay { get; set; }

Reads a bit better to me, particularly in the formatted strings.

responder.RespondDelay = delay;
responder.DelayedActions = RevocationResponder.DelayedActionsFlag.All;


Copy link
Member

Choose a reason for hiding this comment

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

NIt: too many blank lines

@vcsjones vcsjones marked this pull request as draft August 11, 2020 21:11
@bartonjs
Copy link
Member

@vcsjones This portion of the change seems reasonable to me... are you expecting anything else to come in to it soon, or should we just check outerloop and merge this, with followups done in a different PR? (Just getting sensitive about timing)

@vcsjones
Copy link
Member Author

@bartonjs I think outerloop is going to fail; I'm looking at fixing a Windows test today. Should be in by COB.

@vcsjones vcsjones marked this pull request as ready for review August 12, 2020 20:25
@vcsjones
Copy link
Member Author

@bartonjs this is ready for review and and outerloop run, assuming there is nothing else that needs to be addressed immediately.

@vcsjones
Copy link
Member Author

Random thought: we could do it both ways if we wanted to. Windows lets you have a cumulative timeout with CERT_CHAIN_REVOCATION_ACCUMULATIVE_TIMEOUT. Would there be interest in an API that lets folks get the old behavior back on Linux, and use this in Windows? (Post 5, of course)

@bartonjs
Copy link
Member

Would there be interest in an API that lets folks get the old behavior back on Linux, and use this in Windows?

I'm open to the idea, especially if it seems like anyone actually wants it. Clearly it's how I thought the Windows one worked, but early in .NET Core we didn't have the capability of writing the tests reliably (Since it wasn't until after CertificateRequest/AsnReader/AsnWriter/CustomRootTrust that we could do it in a reasonable/self-contained way).

So it's unclear how relevant such an option would be to anyone, but I'm happy to entertain things if we feel it's relevant. (Maybe all 0.6%/12.5%/15.8% want the feature, maybe it's none.)

@vcsjones
Copy link
Member Author

especially if it seems like anyone actually wants it

I have no idea. I'm easily excited by things 7 people on the planet want.

Innerloop failure seems unrelated, I can address any additional feedback Thursday AM.

@bartonjs
Copy link
Member

/azp run runtime-libraries outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vcsjones
Copy link
Member Author

Hmm. Looking at failure.

@bartonjs
Copy link
Member

In case you hadn't had these observations already:

  • The test only failed on Win8.1, it passed on Win10
  • It failed by succeeding the chain, after 1 minute, 30 seconds.

Conclusion: Win8.1 had a bigger max value than Win10.

Solution1: Figure out the Win8.1 limit, and if it's acceptable, change the test to use that on Win8.1.
Solution2: Make the test just exit immediately if running on Windows8.1 or lower.

@vcsjones
Copy link
Member Author

I went with solution 2 in the interest of time, and I at risk of losing power / internet due to weather, so wanted to get something in.

@bartonjs
Copy link
Member

/azp run runtime-libraries outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vcsjones
Copy link
Member Author

None of the failures appear related to this PR.

@bartonjs bartonjs merged commit a24db1c into dotnet:master Aug 13, 2020
@vcsjones vcsjones deleted the impl-38875 branch August 13, 2020 17:33
@karelz karelz added this to the 5.0.0 milestone Aug 18, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Constrain X509ChainPolicy.UrlRetrievalTimeout to acceptable ranges
4 participants