Fix x86 packing issues in System.Data.OleDb

[FreddyDgh]

Fixes #32509

The PR fixes the remaining structure packing problems when running in an x86 process, building upon the work done in #32207. This PR resolves the remaining issues by duplicating the classes in question, but changing the pack to 2 instead of 8. A common interface is also added to both the duplicated and original classes, so that references can be passed around in an architecture-neutral manner.

Edit: this PR has been submitted at the request of @saurabh500.

[dnfclas]

All CLA requirements met.

[saurabh500]

@FreddyD-GH The direction looks good.
How did you verify these changes? Can you add some tests as well based on the discussions that we had in #32509

@jader1313 had mentioned these

command.Connection.ChangeDatabase (dbName);
connection.Database
connection.ServerVersion

And you had mentioned a test case at #32509 (comment)

[saurabh500]

cc @jkotas @maryamariyan
Can you folks take a look at this as well?

[saurabh500]

Also @FreddyD-GH Can you look at the pipeline for the failures.

There is inline initialization of static members and an issue with formatting of OleDbDataReader.cs

OleDbDataReader.cs(2506,18): error SA1028: (NETCORE_ENGINEERING_TELEMETRY=Build) Code should not contain trailing whitespace

OleDb_Util.cs(692,16): error CA1810: (NETCORE_ENGINEERING_TELEMETRY=Build) Initialize all static fields in 'ODB' when those fields are declared and remove the explicit static constructor

[FreddyDgh]

@saurabh500 I have no objections to your changes. However, I do not have all the prerequisites in my environment to build the entire project, so I basically have a smaller project that I can compile and I just copied the changes over. I'm not very well-versed on what Github offers, but would there be an easy way for you to just pickup what I have done and make the necessary changes, so that you can confirm it actually builds properly using your build system?

[saurabh500]

@FreddyD-GH Sounds good. I will take this forward.

[saurabh500]

Also thanks for the contribution. Much appreciated.

[FreddyDgh]

As far as the tests, it's a similar issue, but my tests don't convert over easily. For the tests, I had the one in #32509 for the OleDbConnectionStringBuilder. It looks like there is already a test for command.Connection.ChangeDatabase (dbName);, but must be that it's not being run on x86 in your CI builds?

For connection.Database and connection.ServerVersion, I just did very simple tests along the lines of:

            using (OleDbConnection connection = new OleDbConnection(connStr))
            {
                connection.Open();

                string db = connection.Database;
                string ver = connection.ServerVersion;                
            }

[FreddyDgh]

Not to get ahead of myself, but is there anything special that needs to be done to mark this PR along with #32207 as a servicing candidate? The x86 packing problems should be completely resolved with these changes.

[saurabh500]

The servicing changes need to be taken to the respective servicing branch in the dotnet/corefx repo and then tagged with the appropriate tag to have an offline review initiated.
The PR changes for the servicing branch, along with reasoning behind servicing is then reviewed by the .Net Core Shiproom and after a decision on whether the change makes sense for servicing, it is merged or discarded for the next servicing release.

[jkotas]

Mono failure is known issue #32377

[saurabh500]

I have added the tests to this PR and run the tests on my dev machine in x86 configuration.
The tests were failing with AVs or unexpected errors before the changes and now with the changes in this PR, the tests are passing.

@FreddyD-GH The CI is running tests only on x64 configuration. This is because there were unexpected exceptions when ACE and JET drivers were installed simultaneously. The CI was modified to install only the x64 driver.

Edit:
This PR is ready for merge once the CI is green.

[danmoseley]

@saurabh500 assuming you are considering servicing for this -- it looks like the cutoff for 3.1.4 (may) is 4/14 so I'd bring it no later than a few days earlier.

Of course if you service, the more folks on this issue that can consume the bits out of master and verify they are good, the more likely they are to feel comfortable about approving it

[saurabh500]

@danmosemsft I will target sending the servicing PR by 4/2 i.e. Thursday next week, so that there is some time for validations from the consumers of System.Data.OleDb to build confidence in the change. The servicing PR will have changes from #32207 and from this PR.

@jader1313 @FreddyD-GH @lauxjpn can you folks try the nightly feed and see if the binaries with this change fixes the issues. More details in the comment at #981 (comment)

[lauxjpn]

@saurabh500 I can confirm that the EFCore.Jet test suite of about 15K tests (and configured to use OLE DB) now runs without crash inside an x86 process with the latest preview (5.0.0-preview.3.20178.1) of System.Data.OleDb. 👍

[FreddyDgh]

@saurabh500 I just tried on a project that I tried to convert to .NET Core many months ago and I can confirm that it finally works on x86 as well (using ACE/Jet driver).

Also, someone (that I believe was from Microsoft) posted this link more recently for the nightly NuGet package feed: https://dnceng.pkgs.visualstudio.com/public/_packaging/dotnet5/nuget/v3/index.json
Both @lauxjpn and I have found this one to work better than the one I posted in the comment you referenced.

Perhaps the dotnet/runtime repo docs could be updated to make the correct nightly build feed more discoverable? I don't think that it'd be too crazy to have a small docs section for "Nightly Builds" or even something as simple as: https://github.com/FreddyD-GH/runtime/commit/5ee341c5e44fb06efaedc3c54b4638f90e77b843?diff=split. I know that, for me, when you guys first mentioned a nightly package feed, I found it super-difficult to find an actual URL of the feed mentioned anywhere.

[danmoseley]

Hooray for the positive validation. Have you guys verified that x64 is still working fine?

@FreddyD-GH could you throw up a quick PR with that Readme edit and we can have the right person confirm that's the appropriate feed?

[lauxjpn]

@danmosemsft I can confirm that x64 is working exactly in our tests as before.

(I wouldn't go so far to say it's working fine though. It still suffers from AccessViolationException exceptions as well, but theses are unrelated to this issue and fix here, and were an issue before. They seem to be an issue between OLE DB, ACEOLEDB and ACECORE for x64. See CirrusRedOrg/EntityFrameworkCore.Jet#43 (comment) for further details).

[FreddyDgh]

@danmosemsft I can also confirm that in my other project, both .NET Core x86 and x64 run without issues.

I have also started the requested PR here: #34241

@danmosemsft @saurabh500 My understanding is that your primary goal with the libraries like System.Data.OleDb is to align .NET Core with the behavior of .NET Framework. So, if the issue that @lauxjpn mentions happens on both .NET Core and .NET Framework, do you think anyone would actually take action if we filed a new issue for it on dotnet/runtime? The issue is still non-deterministic, but @lauxjpn has done a good job of isolating the bug and making it reproducible.

[saurabh500]

Thanks for the validations. I will start on the servicing PR.

@FreddyD-GH

So, if the issue that @lauxjpn mentions happens on both .NET Core and .NET Framework, do you think anyone would actually take action if we filed a new issue for it on dotnet/runtime?

If you find an issue please file it. I will look into it.
A standalone repro would be very helpful in these cases like you have provided for all these other issues with OleDb.

[FreddyDgh]

@saurabh500 Did this ever PR (and #32207) ever make it to servicing? I didn't see it in the other repo. Sounds like it may have missed the April 10-15 deadline, which would be unfortunate.

[lauxjpn]

@saurabh500 What is the status on this making it into a servicing release of 3.1?
I already inquired about this in CirrusRedOrg/EntityFrameworkCore.Jet#55 (comment) but never got any answer:

There are some bugs in System.Data.OleDb versions, where method calls are not properly mapped to the actual OLE DB implementation (COM). See dotnet/runtime#33899 and #43. At least most of these should be fixed in very recent builds of System.Data.OleDb. Our tests e.g. run against recent 5.0.0-preview versions of System.Data.OleDb.

This is the underlying issue here. We are using the OleDbConnectionStringBuilder.TryGetValue() method before scaffolding starts, to extract connection string options, so we can use them in subsequent DAO and ADOX calls to retrieve the database schema information (which are needed to navigate around a number of other bugs and missing implementation details) which then is used by the scaffolder.

We could just not use the TryGetValue() method, but if this version of System.Data.OleDb would be used when running EF Core queries, other unrecoverable bugs would surface, so we will not implement a fix for this. Instead, a version of System.Data.OleDb where this bugs have been fixed needs to be used.

Looks like the PR by @FreddyD-GH (dotnet/runtime#33899) still hasn't made it into a public release.

@saurabh500 @danmosemsft Shouldn't have this gone into a servicing release months ago?

(From dotnet/runtime#33899 (comment))

Thanks for the validations. I will start on the servicing PR.

@FreddyD-GH already asked about this on April 15:

@saurabh500 Did this ever PR (and #32207) ever make it to servicing? I didn't see it in the other repo. Sounds like it may have missed the April 10-15 deadline, which would be unfortunate.

[lauxjpn]

Well, with .NET 5 being released tomorrow, I guess we will now be able to use the 5.0 version of System.Data.OleDb (which is .NET Standard 2.0 compliant) after trying to get in touch with @saurabh500 (or anyone really) for 6 month without a single answer.

While the .NET 5.0 release of System.Data.OleDb will finally resolve the issue at hand, this has been a needlessly disappointing experience for us.

I would really like to know what happened there.

/cc @danmosemsft

[danmoseley]

@lauxjpn I think the problem was is that ownership of this moved to the team of @ajcvickers since this PR was closed, and he was not tagged on this closed PR so he didn't see it. I was tagged, but didn't notice - sorry about that.

As a general rule you can find the owners of an area here https://github.com/dotnet/runtime/blob/master/docs/area-owners.md and tag them directly. Also it often helps to open a new issue, rather than post on a closed PR or issue. Still, we should have noticed.

[lauxjpn]

@danmosemsft I had no idea that Arthur is in charge now. This might make the System.Data related stuff easier for our EF Core related Jet provider (https://github.com/bubibubi/EntityFrameworkCore.Jet/) in the future.

Thanks for the link about the area owner page. Might become handy in the future.

[ajcvickers]

@lauxjpn Bit swamped right now, but let's get together with @roji and @JeremyLikness about this specifically, but also how you are using OleDb generally and with EF Core.

[lauxjpn]

@ajcvickers Yeah, that is a good idea. I am currently also more concentrated on our Pomelo release for EF Core 5 than our Jet release for EF Core 3.1. How is next month for you? The immediate issue we had here is resolved with the 5.0 release anyway (though there remain other OLE DB, ODBC, ADO and DAO related issues, but not all necessarily reported yet).

[ajcvickers]

@lauxjpn Next month would be great. I'll keep a note on my backlog.

[lauxjpn]

@ajcvickers Can you get me in touch with someone who maintains the Jet/Access ODBC driver/provider?
We came across an issue and are not sure how to solve it, because the docs seem to be lacking in this regard:

• How to specify MDW username and password (user-level security) AND the database password (shared-level security) in an ODBC connection string?
• Related to Fix connection string conversion from ODBC to OLE DB CirrusRedOrg/EntityFrameworkCore.Jet#72

[ajcvickers]

@lauxjpn My team owns the ADO.NET providers for OleDb and ODBC. That being said, we are not experts in this area and these providers are basically in maintenance mode. My understanding is that there are many inconsistences and unsupported scenarios for these providers, and at first glance this looks like something that isn't supported by ODBC. It's not totally inconceivable that we could take a PR to add support, but it would need to be driven by the community and would need to be non-breaking for existing apps.

[lauxjpn]

@ajcvickers I believe the issue is one or two levels down the rabbit hole, either in the native ODBC implementation or in the Jet ODBC driver. It's not so much that we want to fix it, its more a question of, whether we missed something or there exists an option that is not documented.

The things we are interested in in regards to the Jet ODBC driver are:

• Is the PWD connection string option actually the sole option for specifying a password?
  • If so, how to specify a workgroup user password and a database password simultaneously?
    • If this is not possible when using ODBC (it appears to be for OLE DB), what is the algorithm that is used internally to determine when a PWD option is used as the password for the workgroup user and when as the password for the database itself?
  • If not, what is the other option, and which is used for the workgroup user password and which for the database password?

[ajcvickers]

@lauxjpn Let's get on a call and discuss this and ODBC in general. Can you email with availability for next week, or whenever works for you? /cc @JeremyLikness @roji @bricelam