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.

Sign up for GitHub

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

Jump to bottom

Send window updates based on examined rather than consumed. #8200

Merged
merged 12 commits into from
Mar 13, 2019
Merged

Send window updates based on examined rather than consumed. #8200

merged 12 commits into from
Mar 13, 2019

Conversation

jkotalik
Copy link
Contributor

@jkotalik jkotalik commented Mar 5, 2019

Fixes: #8105

Will merge #8198 separate from this change (the other half).

@jkotalik
Copy link
Contributor Author

jkotalik commented Mar 5, 2019

Need #8198 before this can be merged.

@jkotalik jkotalik added this to the 3.0.0-preview4 milestone Mar 5, 2019
@jkotalik jkotalik mentioned this pull request Mar 5, 2019
Closed
@analogrelay
Copy link
Contributor

/azp run AspNetCore-helix-test

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jkotalik
Copy link
Contributor Author

jkotalik commented Mar 6, 2019

@halter73 @Tratcher I updated the logic. I think this works. I'll add tests appropriately once we confirm my logic makes sense.

@jkotalik jkotalik force-pushed the jkotalik/UseExamineHttp2 branch from 3998595 to 1ce8e44 Compare March 6, 2019 19:40
@jkotalik
Copy link
Contributor Author

jkotalik commented Mar 6, 2019

@aspnet/brt @mikaelm12 latest macOS failure.

  com.microsoft.signalr.HubConnectionTest > TransportAllUsesLongPollingWhenServerOnlySupportLongPolling() FAILED
      java.lang.RuntimeException at HubConnectionTest.java:1245
          Caused by: java.lang.Exception at HubConnectionTest.java:1245

@mikaelm12
Copy link
Contributor

latest macOS failure.

Noted. I'll file an issue and fix it. Thanks

@jkotalik jkotalik marked this pull request as ready for review March 8, 2019 00:24
@jkotalik
Copy link
Contributor Author

jkotalik commented Mar 8, 2019

Test failure is with InMemory process crash again. :/.

Besides that 🆙 📅

@jkotalik jkotalik closed this Mar 8, 2019
@jkotalik jkotalik reopened this Mar 8, 2019
@jkotalik jkotalik mentioned this pull request Mar 8, 2019
Closed
@jkotalik jkotalik force-pushed the jkotalik/UseExamineHttp2 branch from 7104b76 to 897e275 Compare March 8, 2019 18:19
@jkotalik
Copy link
Contributor Author

jkotalik commented Mar 8, 2019 •
edited
Loading

@aspnet/brt MacOS timeout near razor component stuff. I've seen this happen twice now. Someone needs to look into timeouts <_<

        Request finished in 29.343400000000003ms 302 
  WARNING: Could not set default HttpMessageHandler because 'GetHttpMessageHandler' was not found on 'System.Net.Http.HttpClient'.
  WARNING: Could not set default HttpMessageHandler because 'GetHttpMessageHandler' was not found on 'System.Net.Http.HttpClient'.
  [xUnit.net 00:00:03.31]     Microsoft.AspNetCore.Blazor.Build.Test.ComponentRenderingRazorIntegrationTest.Render_HtmlBlock_Integration [SKIP]
  Skipped  Microsoft.AspNetCore.Blazor.Build.Test.ComponentRenderingRazorIntegrationTest.Render_HtmlBlock_Integration
  [xUnit.net 00:00:03.70]     Microsoft.AspNetCore.Blazor.Build.Test.ComponentRenderingRazorIntegrationTest.Render_Component_HtmlEncoded [SKIP]
  Skipped  Microsoft.AspNetCore.Blazor.Build.Test.ComponentRenderingRazorIntegrationTest.Render_Component_HtmlEncoded
  [xUnit.net 00:00:00.79]     Templates.Test.SpaTemplateTest.ReactReduxTemplateTest.ReactReduxTemplate_Works_NetCore [SKIP]
  Skipped  Templates.Test.SpaTemplateTest.ReactReduxTemplateTest.ReactReduxTemplate_Works_NetCore
  [xUnit.net 00:00:37.24]     Templates.Test.SpaTemplateTest.AngularTemplateTest.AngularTemplate_Works [SKIP]
  [xUnit.net 00:00:37.24]     Templates.Test.MvcTemplateTest.MvcTemplate_NoAuthImpl(languageOverride: "F#") [SKIP]
  Skipped  Templates.Test.SpaTemplateTest.AngularTemplateTest.AngularTemplate_Works
  Skipped  Templates.Test.MvcTemplateTest.MvcTemplate_NoAuthImpl(languageOverride: "F#")
  [xUnit.net 00:00:49.15]     Templates.Test.SpaTemplateTest.ReactTemplateTest.ReactTemplate_Works_NetCore [SKIP]
  Skipped  Templates.Test.SpaTemplateTest.ReactTemplateTest.ReactTemplate_Works_NetCore
##[error]The operation was canceled.
##[section]Finishing: run ./eng/scripts/cibuild.sh

davidfowl
davidfowl reviewed Mar 8, 2019
View reviewed changes
src/Servers/Kestrel/Core/src/Internal/Http2/Http2MessageBody.cs
@@ -63,14 +64,57 @@ public override void AdvanceTo(SequencePosition consumed)

public override void AdvanceTo(SequencePosition consumed, SequencePosition examined)
{
var dataLength = _readResult.Buffer.Slice(_readResult.Buffer.Start, consumed).Length;
// This code path is fairly hard to understand so let's break it down with an example
Copy link
Member

Choose a reason for hiding this comment

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

ZOMG the performance :sad:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would you fix it? :)

Copy link
Member

Choose a reason for hiding this comment

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

Measure first

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't have any in memory profiling for HTTP2, if you require it for this PR, I'll do it, but it adds a decent amount of work.

Copy link
Member

Choose a reason for hiding this comment

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

OK we don’t need to gate on that.

@analogrelay
Copy link
Contributor

Yeah, we have some thoughts on timeouts, we'll be talking about this on Monday I think.

@jtattermusch jtattermusch mentioned this pull request Mar 11, 2019
Merged
@jkotalik jkotalik closed this Mar 12, 2019
@jkotalik jkotalik reopened this Mar 12, 2019
@jkotalik
Copy link
Contributor Author

Port conflict issue with ANCM OutOfProc with music store. https://dev.azure.com/dnceng/public/_build/results?buildId=121641

@jkotalik
Copy link
Contributor Author

@halter73 @davidfowl when you get the chance, can you hook me up with an approval 😄

@jkotalik jkotalik merged commit 17d072f into master Mar 13, 2019
@jkotalik jkotalik deleted the jkotalik/UseExamineHttp2 branch March 13, 2019 05:00
@jtattermusch jtattermusch mentioned this pull request Mar 14, 2019
Merged
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidfowl davidfowl davidfowl left review comments

@Tratcher Tratcher Tratcher left review comments

@halter73 halter73 halter73 approved these changes

Assignees
No one assigned
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Milestone
3.0.0-preview4
Development

Successfully merging this pull request may close these issues.
8 participants
@jkotalik @analogrelay @mikaelm12 @halter73 @davidfowl @Tratcher @Eilon @amcasey