Uh oh!
There was an error while loading. Please reload this page.
refactor old APM [Begin/End]Accept methods on top of Task APIs, and enable for Unix as well - #51212
Conversation
ghost
commented
Apr 14, 2021
Tagging subscribers to this area: @dotnet/ncl Issue DetailsNote, the old Begin/EndAccept methods support doing an accept and receive in a single operation. Unfortunately the API for this is terrible and forces allocation. It's also not currently supported on non-Windows platforms. So, replace this with a helper routine that performs an accept followed by a receive, and works across all platforms. Contributes to #43845
|
geoffkizer
commented
Apr 14, 2021
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
stephentoub
commented
Apr 14, 2021
In theory there's a perf benefit possible from the existing accept+receive on Windows, but as we only expose it from an old APM-based API we discourage using, it incurs non-trivial allocation overheads, and it's complicated, non-portable logic, I'm fine seeing it go away. |
Uh oh!
There was an error while loading. Please reload this page.
geoffkizer
commented
Apr 18, 2021
I also pushed a change to remove the CallbackClosure cache. This is now only used for BeginSendFile, and that will go away eventually as well. Saves 8 bytes on the Socket object. |
geoffkizer
commented
Apr 18, 2021
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…nable for Unix as well
geoffkizer
commented
Apr 18, 2021
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
geoffkizer
commented
Apr 18, 2021
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| return null; // unreachable | ||
| } | ||
| public IAsyncResult BeginAccept(AsyncCallback? callback, object? state) => | ||
| TaskToApmBeginWithSyncExceptions(AcceptAsync(), callback, state); |
There was a problem hiding this comment.
When was this added? I don't remember seeing it. Is this actually providing compatibility with what was there before? It's not just throwing exceptions that may have occurred on the initiation path, it's throwing all exceptions from the whole operation if it happens to complete quickly, by the time we get to the IsFaulted check. I don't have a strong opinion, but if our goal is compatibility, i think this likely makes it worse, potentially throwing new exceptions that previously weren't possible from Begin. I suggest we just accept the small difference in behavior we've accepted everywhere else we've used TaskToApm, rather than trying to special-case this. If you want to keep it, though, ok.
There was a problem hiding this comment.
This got added in #51213 (which I just merged last night) to address the compat concerns from @antonfirsov. I didn't think it would be controversial, so I merged it without additional review.
It provides compat with what was there before in the sense that sync exceptions now propagate synchronously. It also, as you point out, will throw synchronously if the task completes asynchronously but before we hit the IsFaulted check. AFAIK there is no way to avoid this (right?).
I suggest we just accept the small difference in behavior we've accepted everywhere else we've used TaskToApm, rather than trying to special-case this.
I'm fine with this. I'll just rip this code out. @antonfirsov any concerns here? Based on this I think we should close #47905 without action as well.
There was a problem hiding this comment.
@stephentoub le me know, if my definition of breaking changes, and/or my expectations around maintaining docs are too strict, or if I'm misunderstanding the rules in other means, but this is how I view it:
AFAIK we do not document async exceptions the same way we do with we sync exceptions since they are not being actually thrown by calling the method, but rather by awaiting it.
This means that if we close #47905 as wontfix, we need to add a "breaking change" label to all these PR-s, and make sure we propagate and address all the related documentation work in 6.0. If the goal is to save time, I'm don't think this path will result in less work. I would just add a tiny layer of compat code instead.
There was a problem hiding this comment.
Whether the behavior was documented or not, it can still be a breaking change. And my point is that in this case, it's a breaking change whether or not this TaskToApmBeginWithSyncExceptions is used, and from my perspective, it's a larger break with TaskToApmBeginWithSyncExceptions (it's non-deterministically causing more exceptions to be thrown out of the BeginXx method). I'm not convinced either way it rises to the level that requires being documented as a breaking change, but if you feel it needs to be, feel free. To my knowledge we haven't in the past when we've ripped out APM implementations and replaced them with TaskToApm wrappers around XxAsync methods.
There was a problem hiding this comment.
I'm not convinced either way it rises to the level that requires being documented as a breaking change
Do we have any formal guidelines about the bar for breaking changes?
My main concern is around the API docs. What is our process then for making sure that the exceptions which are no longer thrown synchronously will be removed from the list? If there is none, we will create inconsistency in our docs which doesn't look good, even if these particular API-s are marginal.
There was a problem hiding this comment.
We can still accomplish it with a trick like the one below
Can you share a full example? I think it's much more complicated than that, and would deoptimize the XxAsync methods we're trying to reuse.
There was a problem hiding this comment.
Thanks. I left a few comments. That only partially addresses the issue, and adds non-trivial complication. I don't believe it's worth it. If you believe this rises to the level of requiring a breaking change notification, please feel free to do so. But FYI we've made such changes throughout .NET Core incrementally over the last several years, ripping out lots of custom IAsyncResult code across lots of libraries, and I don't believe we've documented any of them as being breaking.
There was a problem hiding this comment.
Thanks! I see now the more complicated cases, and agree with your analysis, that it's not worth it.
ripping out lots of custom IAsyncResult code across lots of libraries, and I don't believe we've documented any of them as being breaking.
When it comes to the process that is triggered by applying the "breaking change" labels today, I don't think it makes sense to deviate from an established practice in this particular case of sockets. I opened #6658 to track the API docs work.
There was a problem hiding this comment.
I opened #6658 to track the API docs work.
This looks like it's linked to the wrong repo, perhaps?
geoffkizer
commented
Apr 18, 2021
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| public IAsyncResult BeginDisconnect(bool reuseSocket, AsyncCallback? callback, object? state) => | ||
| TaskToApmBeginWithSyncExceptions(DisconnectAsync(reuseSocket).AsTask(), callback, state); | ||
| TaskToApm.Begin(DisconnectAsync(reuseSocket).AsTask(), callback, state); |
There was a problem hiding this comment.
I don't understand how does this work without TaskToApmBeginWithSyncExceptions now.
There was a problem hiding this comment.
Ah ok, missed the discussion above.
| server.BindToAnonymousPort(IPAddress.Loopback); | ||
| Assert.Throws<InvalidOperationException>(() => { AcceptAsync(listener, server); }); | ||
| await Assert.ThrowsAsync<InvalidOperationException>(() => AcceptAsync(listener, server)); |
There was a problem hiding this comment.
In case of APM, shouldn't this throw without awaiting?
Note, the old Begin/EndAccept methods support doing an accept and receive in a single operation. Unfortunately the API for this is terrible and forces allocation. It's also not currently supported on non-Windows platforms. So, replace this with a helper routine that performs an accept followed by a receive, and works across all platforms.
Contributes to #43845