Implement DbDataSource

[roji]

Closes #64812

See naming discussion in #64812 (comment)

[dotnet-issue-labeler]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

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

Issue Details

---

This is a WIP PR for DbDataSource (see #64812), there's still some work to be done here.

Closes #64812

With regards to the API review comments (#64812 (comment)):

Maybe GetConnection() should be called CreateConnection() because it returns a new instance

The current naming in the PR for getting/renting a connection is CreateConnection (for closed) and CreateOpenConnection (for open, as a convenience).

• I'm not very happy with those names.
• The original GetConnection/OpenConnection were possibly confusing since they imply there's just one connection, and these methods return it.
• On the other hand, most DbDataSource instances will pool, and the naming "Create" could be confusing in such a pooling context.
• Another suggestion was CreateConnection/OpenNewConnection, which has a similar issue (and is possibly even stronger in suggesting that a new physical connection is being created, which wouldn't be good).

Consider introducing a term that represents connection who logically don't have a connection and rename CreateCommand()

• The issue is that the instance returned from CreateCommand doesn't allow accessing the Connection and Transaction properties (since it owns the connection and manages it internally).
• After thinking about it, I currently think leaving the name CreateCommand is the right thing to do:
  • Manipulating these two properties is only a small part of the command API surface - execution is the main function, and that behaves just like a normal command. This command doesn't behave fundamentally differently from a regular command - it just has two disabled properties.
  • There are already command states in which e.g. mutating Connection/Transaction shouldn't work, e.g. changing them while the command is in progress and a reader is open.
  • Introducing a new special term would add more confusion than help; an informative NotSupportedException from these properties should be sufficient.

Author:	roji
Assignees:	-
Labels:	area-System.Data, new-api-needs-documentation
Milestone:	-

[KalleOlaviNiemitalo]

There might not be an original exception.

[roji]

That's true - I had an offline discussion with @NinoFloris about whether Close exceptions should generally bubble up.

On the one hand, Close failures aren't really relevant to the execution itself, which completed successfully - which is why the current code doesn't (ever) bubble them up (the idea behind this API is that the user doesn't care about connection-related details any more). It's a bit comparable to what happens if an internal logging failure occurs in some library; usually you don't want that to be bubbled up to the user, since logging is secondary and failure there doesn't affect the main operation.

On the other hand, Open exceptions do bubble up (since we can't execute), so it's maybe somewhat inconsistent.

I'm still leaning to not bubbling it up (updating comment to reflect the reasoning), but am interested in opinions to the contrary.

[KalleOlaviNiemitalo]

Should it log the swallowed exceptions to DataCommonEventSource? There is ExceptionBuilder.TraceExceptionWithoutRethrow for that purpose, but it seems to be used in DataTable-related classes only.

[roji]

Good suggestion... I suspect it's only currently used in DataTable-related classes is that the main System.Data classes are abstractions only, and therefore don't need to swallow/report exceptions. I'll make that change - it would allow visibility to anyone who's really interested in these exceptions, and I don't see a downside to it.

[KalleOlaviNiemitalo]

The commit message is "Trace swallowed exceptions and fix comments" but it doesn't add any tracing.

[roji]

Pushed it

[bgrainger]

In some providers (e.g., MySqlConnector), Prepare(Async) can only be called if the command is associated with an open connection. (In MySQL Server, a prepared command is strongly associated with just one server session, so it needs to be known which session the command should be prepared on. Moreover, it doesn't make sense to open a connection, prepare the command, then close the connection without executing the command.)

So this method as written will fail with MySqlConnector. (And maybe other providers, too; I haven't checked them.)

Maybe that's MySqlConnector's problem to deal with, and it should either document that failure as a "known issue", or implement its own DbCommandWrapper that opens the connection in Prepare, keeps it open, and reuses that open connection in ExecuteXyz?

For now, I don't think the core logic in DbDataSource needs to be adjusted to accommodate this, but simply raising the issue for awareness, in case it is very common across most ADO.NET providers.

(The workaround I envision would be setting a _isOpen field to true in Prepare, then checking that field in other methods so Open isn't called twice. However, this is probably overkill since calling Prepare on a one-shot command is almost certainly pointless in most use cases.)

[bgrainger]

Alternatively, throw ExceptionBuilder.NotSupportedOnDataSourceCommand(); in this implementation, and let providers opt into connectionless Prepare (via a specialised class) if they can support it. This approach might make sense if most providers can't support this.

[roji]

@bgrainger yeah, when I originally thought about this, I thought the base implementation should just delegate and let the provider throw, which is what most providers do (SqlClient doesn't throw for Prepare without a connection, but that seems to just be a no-op, which isn't useful behavior either). But the provider exception ("no open connection") is likely to be a bit cryptic in this context, and ExceptionBuilder.NotSupportedOnDataSourceCommand seems more informative.

In the DbDataSource world, Prepare on a connectionless command could mean "prepare this on the server in general", for databases where prepared statements aren't bound to specific connections (FWIW PostgreSQL is the same as MySQL in this respect, and AFAIK most databases). So there's at least some reasonable implementation for this - though it would definitely require a specific data-source (connectionless) implementation, and simple delegation wouldn't work out-of-the-box.

Another way to look at this (as you hint), is to open a connection and keep it open until the command is disposed. This would allow multiple prepared executions via the same "connectionless" command without having to deal with a connection, but it goes against the connectionless model we're introduced here. Another issue here is that it's (unfortunately) fairly common for users (and even ORMs, specifically EF6) to not dispose commands; in this case, it would mean that the connection is never disposed, causing a leak. I'd probably avoid this just for that.

So I'll change the default implement to throw ExceptionBuilder.NotSupportedOnDataSourceCommand() as you suggest; providers with connectionless prepare can indeed override and implement their logic in any case.

[roji]

Thanks for the feedback @bgrainger, addressed your comments - let me know if you have anything else.

[ajcvickers]

🐑:it:

[GSPP]

@roji I don't know if this is still relevant, but here we go: One reason to bubble up all exceptions (if at all possible) is that developers need to be alerted of problems. Like, if there is network flakiness or some server issue, it needs to show up in error reporting. Yes, it might not make a difference during normal execution but there is still something wrong if Close fails.

There could also be internal bugs to some library or provider that better show up in error logs. There could be rare state corruption going on, and swallowing would hinder debugging efforts.

[roji]

@GSPP definitely relevant - we can still change stuff here until the release.

One reason to bubble up all exceptions (if at all possible) is that developers need to be alerted of problems. Like, if there is network flakiness or some server issue, it needs to show up in error reporting. Yes, it might not make a difference during normal execution but there is still something wrong if Close fails.

Right, and that's why these problems are logged - non-critical issues which don't hinder successful execution don't necessarily have to be raised as exceptions, which interrupt the actual user experience (e.g. fail a web request although the database operation did execute successfully).

A good comparison here is what happens when calling DbConnection.Dispose; it's supposed to close an open connection, but Dispose isn't supposed to surface any errors (in general in .NET).