React to runtime/594 Encoder.Convert change#17747
Conversation
|
👀 |
src/Http/Http.Abstractions/src/Extensions/HttpResponseWritingExtensions.cs
Outdated
Show resolved
Hide resolved
0a963e0 to
0681695
Compare
|
@aspnet/build we've done away with patchconfig stuff, right? |
|
@GrabYourPitchforks I thought you had detailed two affected locations? |
@Pilchie we reviewed the other two uses of this API and determined that they were not affected by the change. |
|
@Pilchie I believe this is the only location that would be affected by the 3.1.1 change. I had called out another location that has a buggy call site, but that second call site is buggy regardless of which runtime it's running against. The 3.1.1 change won't affect it. |
We've done so in 3.0, but haven't merged that forward to 3.1 yet: https://github.com/aspnet/AspNetCore/blob/9a4ab80f7e068e30da5ea3c819b546b511b64302/Directory.Build.targets#L61-L64 We intend to merge it forward with the rest of #17311 (CC @dougbu @JunTaoLuo) |
|
@mmitche Any reason not to merge this? @GrabYourPitchforks are the other changes ready/merged? |
They aren't ready yet. I think we should hold those changes for 3.1.2. |
|
@Pilchie is this approved? It still has servicing-consider, so I've put it in 3.1.x since we generally don't put things in a specific patch milestone until they're approved. |
|
Not approved for 3.1.1 |
|
It's been conceptually approved at Tactics, but that was before the PR was actually created. However, as @mmitche says, we'll hold it from 3.1.1 at this point as the base changes aren't going to make 3.1.1. |
I guess my question is this: Does tactics need to further review this change? If no, we can mark |
|
I'm going to bring up the slip to 3.1.2 in Tactics tomorrow to make sure everyone is aware. Once that happens, we'll move to |
|
Putting |
|
Approved for 3.1.2 |
|
Moved to Mar as per Tactics. Will need to be explicitly approved for Feb, if required. |
|
@anurse I believe this got bumped back to feb now. Please confirm when you have the chance 😄 |
|
Correct, but @Tratcher I believe this is reacting to a change that hasn't actually happened in runtime's 3.1.x branch. Can you check on that? |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
dotnet/runtime#594 is reverting a 3.1 fix to encoders for compat reasons. That might have side-effects for our use of encoders so we're pre-emptively updating AspNetCore as well.
This change works because we know WriteMultiSegmentEncoded is always called with a complete string, there's no tearing, so we can always flush the encoder.