Added a physical open callback#3678
Conversation
NinoFloris
left a comment
NinoFloris
left a comment
There was a problem hiding this comment.
LGTM, thanks a lot!
One thing you might add is a test for this callback + multiplexing. Today as you wrote it everything works ok, tomorrow it could be moved around by more pooling code and end up running after the connection is already available for use by other multiplexing callers.
Co-authored-by: Emmanuel André <[email protected]>
roji
left a comment
roji
left a comment
There was a problem hiding this comment.
Looks good! See mainly nits below.
I wonder how many people will ever use this aside from @NinoFloris 🤣
Co-authored-by: Shay Rojansky <[email protected]>
The main thing is, it makes sense, there is almost no perf downside and it's easy to implement, so why not 😛 |
| throw new InvalidOperationException($"Asynchronous {nameof(PhysicalOpenCallback)} isn't supported with synchronous {nameof(NpgsqlConnection.Open)}"); | ||
| } | ||
|
|
||
| await physicalOpenTask; |
There was a problem hiding this comment.
Small nit: if we're in sync mode, we still await the task - this is OK since we've checked above that it's completed. But we could just skip that altogether. So, if the task is complete, no need to do anything (in either sync or async), if it's incomplete we await it in async and throw in sync.
There was a problem hiding this comment.
We actually should for ValueTask ;)
There was a problem hiding this comment.
That's true, but it's a Task now :)
Another reason why I prefer using Task rather than ValueTask everywhere by default. The contract for Task is much simpler and less bug-prone, and when there's really no perf justification for it...
There was a problem hiding this comment.
Ah I hadn't seen that change, yes for task it can really just short circuit!
There was a problem hiding this comment.
Totally off-topic, but I never understood why they chose to integrate IValueTaskSource into ValueTask as opposed to create a new awaitable. That would have made it typingly obvious what a method returns and what you're allowed to do to it. The non-IValueTaskSource ValueTask (which either contains the value or wraps a Task) is pretty simple and can also be reused without danger, but you can never be sure what's actually behind a ValueTask...
There was a problem hiding this comment.
Bit of the issue with the .NET type system is you have no nice existential return type support (without lifting it visibly into the type as a universal type). So you can either return a concrete struct or always pay for an interface. GetAwaiter for task already returned the concrete TaskAwaitable struct so I don't see they had much choice.
If you want to be sure you can do whatever with a valuetask you can always call Preserve, and it's nicely short circuited when it already wraps a task.
There was a problem hiding this comment.
But I mean, why could they not have created a public ReusableValueTask (or whatever name), which is the awaitable wrapping IValueTaskSource, instead of folding that functionality into the existing ValueTask, whose purpose at that time was only to optimize synchronous completion?
There was a problem hiding this comment.
I know Stephen Toub really doesn't like having too many user visible types, and I can imagine ValueTask & Task is already concept heavy for many users.
roji
left a comment
roji
left a comment
There was a problem hiding this comment.
Super small nit if you feel like it.. LGTM, no need for another review from me.
|
Final question, how would the PhysicalOpenCallback make sure it can support both sync and async open? Should it just do blocking queries in that case? As it does not know when called if awaiting anything is actually supported. |
I think the idea is that in a single application, you either use sync or async, and so you provide the appropriate callback. Do you have another scenario in mind? Otherwise we could have two callbacks - though that may be a bit heavy... |
Or we can pass the boolean |
|
Yeah for us it's completely fine but you don't always control the callers of DbConnection methods, say in health checks. Just posed it as a question so we have at least thought about it. |
We could, but the boolean async value is an internal implementation detail of Npgsql (in order to unify sync/async paths)... for public APIs, we always expose two entry points as is standard in .NET. Thinking about it a bit more, it does seem more standard-conformant to just expose two callbacks... This would also obviate the sync-over-async issue altogether. |
|
@roji going to ping you yet again as I've added another callback. |
4 participants
Closes #3032