Add partial packet detection and fixup #2714
Conversation
|
I've added comments to the Packet class as requested. The CI was green apart from some ubuntu legs which timed out, many other ubuntu legs succeeded so I don't see any direct inference on from that. Ready for review @David-Engel @saurabh500 @cheenamalhotra |
|
@Wraith2 We are reviewing this and hope to get faster traction towards EOW. |
|
Pasting test failure for reference: This test should be looked at carefully. It failed on Ubuntu with .NET 6 and 8 , and also hung up on Windows when ran with Managed SNI, link to logs 1 link to logs 2. @Wraith2 can you confirm if this is something you're able to repro in Windows with Managed SNI? Please make sure config file is configured to enable Managed SNI on Windows. |
| { | ||
| // Do nothing with callback if closed or broken and error not 0 - callback can occur | ||
| // after connection has been closed. PROBLEM IN NETLIB - DESIGN FLAW. | ||
| return; |
There was a problem hiding this comment.
Can you add a Debug Assert here and check if this is taking any hit?
|
Isn't this the set of tests that @David-Engel pointed out in #2608 (comment) ? If so we discussed it at length on the teams call. I don't believe that those tests are reliable. Setup a breakpoint or Debug.WriteLine where an exception is added to the state object and run the test. You should find that an exception is always added to the state object but that the test will usually succeed. That should not be possible, an exception if added should be thrown. The test is missing failures and if that's the case then the test is unreliable. |
|
When you work past the terrible code in SNITCPHandle and make the test run for long enough it settles into a steady state where it can't reach the end. There is no indication why yet. those 9 in flight items just don't seem to complete but i don't know why. |
|
After a few more hours investigation I know what the problem is but I have no clue what change has caused it. In SqlDataReader when an async method is called we use a context object to contain some state and pass that context object to all the async methods that are used to implement the async read machinery. Part of this state is the TaskCompletionSource. What I don't understand is how cancel is supposed to work. I'm unable to run the tests in native sni mode because the native sni can't be initialized (can't find the sni dll). So I can't compare the managed to unmanaged implementations here. I don't believe that I have made any change that should affect cancellation. I have verified that there are no partial packets in the state objects when the async tasks get stuck. I don't understand how async cancellation is supposed to work at all. |
Wraith2
force-pushed
the
operation-status-part2
branch
from
88fbb93 to
8b57818
Compare
|
Can someone with CI access rerun the failed legs? the failures are random or CI resources not being available as far as i can tell. |
|
The current failures are interesting. They're in the test that was failing before but they new ones are only detected because i made the test more accurate. The previous version of the test accepted any exception when it was expecting a cancellation exception. It was passing on netfx with my previous changes because timeout exceptions were being thrown. I judged that accepting a timeout when we were supposed to be testing whether cancellation had occurred was not correct. If we retained the previous version of the test then everything would have passed cleanly. In the current situation since the test completed correctly without hanging the result is equivalent to what we would have experienced in all test runs in the past, all started threads that we expected to be cancelled exited with an exception. [edit] |
|
@Wraith2 You might be banging your head against an unrelated issue in the driver. IIRC, the test was only introduced to ensure we don't regress "The MARS TDS header contained errors." issue. (The test code came from the repro.) If you isolate your test changes and run them against main code, does it still fail? Yes, the correct exception is probably "Operation cancelled by user." where the exception is being caught. But if it's unrelated to your other changes, I would leave that part of the test as it was and file a new issue with repro code. As it is, it's unclear if and how this behavior is impacting users and I wouldn't hold up your perf improvements for it. |
|
There was definitely a real problem. The results differed between main and my branch. I've solved that issue now and the current state is that we're seeing a real failure because I've made the test more sensitive. I think it's probably safe to lower the sensitivity of the test again now because the new test that I've added covers the specific scenario in the multiplexer that I had missed and everything else is pass. I'll try that and see how the CI likes it. I think the current state on this branch is that it is as stable as live. We need to have confidence that this set of changes is correct before we can merge it. It's high risk and high complexity code. Even understanding it very deeply it has taken me a week to actively debug a very important behaviour change that I missed. |
|
Can someone re-run the failed legs? the only failing test is something to do with event counters which I've been no-where near. |
|
The failing test is EventCounter_ReclaimedConnectionsCounter_Functional. It's doing something with GC specific to net6. It's failing sporadically on net6 managed sni runs but not deterministically. I can't make it fail locally to trace what might be happening. |
|
Any thoughts? |
|
I'm not seeing the failures you mentioned in EventCounter_ReclaimedConnectionsCounter_Functional [in the CI results]. I mainly see fairly consistent failures of CancelAsyncConnections on Linux. It seems to pass on Windows managed SNI, so there might be something that is Linux/Ubuntu network specific. Can you run/debug the test against a local WSL or Docker instance? |
|
If i click through the failure i get to this page https://sqlclientdrivers.visualstudio.com/public/_build/results?buildId=95784&view=ms.vss-test-web.build-test-results-tab The cancel tests are passing now, those failed in the previous runs but not the current ones. |
|
I think there is something wrong with the Tests tab. I don't usually reference it. I scroll down the summary tab to see what jobs had failures:
Then drill into the job and the task that failed to see the log:
|
|
If it's AsyncCancelledConnectionsTest again then there isn't anything further I can do. That test is multithreaded and timing dependent. I've traced the individual packets through the entire call stack. I've run it for 1000 iterations successfully after fixing a reproducible error in it. If someone can isolate a reproducible problem from it then i'll investigate. |
|
I chatted with @saurabh500 and I just want to add that this is definitely something we all want to see get merged. It'll just take someone finding time (could take a few days dedicated time) to get their head wrapped around the new code and be able to help repro/debug to find the issue. |
|
I'm happy to make myself available to talk through the code with anyone that needs it. |
|
@Wraith2 and @David-Engel I was looking at the lifecycle of the snapshots and something that stood out in NetCore vs NetFx is that SqlDataReader for NetCore is storing the cached snapshot with the SqlInternalConnectionTds which is a shared resource among all the SqlDataReader(s) running on a MARS connection. This means that we are saving the reader snapshot on the shared resource, which can be overwritten by any other reader. @Wraith2 have you had a chance to pursue this line of investigation for hanging test? I wonder if the timing is causing the wrong cached snapshot to be provided to a SqlDataReader, causing data corruption and likely causing a hang. |
|
SqlInternalConnection.cs |
|
@Wraith2 I see that you had made the changes in the first place. Can you try another PR where you remove the storage of these contexts and snapshots on SqlInternalConnection and with the multiplexing change, try to see if this solves the problem. Also, I am Happy to be told that my theory is wrong, but I would like to understand how in MARS cases, the shared Cached contexts on InternalConnection is a safe design choice. |
cheenamalhotra
requested review from
imasud00,
benrr101,
mdaigle and
samsharma2700
|
Just checking where this is up to. I see we've got a several new reviewers. If you want a teams call to talk through what I've done just let me know. |
There was a problem hiding this comment.
Now that we've branched 6.0, this should be mergeable. We definitely want to get this in sooner than later for 7.0 preview 1. I'm good with the changes with the addition of the license header to one file and addressing conflicts. We will just need a second after that.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/FunctionalTests/MultiplexerTests.cs
Outdated
Show resolved
Hide resolved
David-Engel
dismissed
cheenamalhotra’s stale review
Changes have been addressed
|
Hi @Wraith2, thanks for the detailed writeups and design notes. I really appreciate your thorough approach to this. I'm working through this PR to add my review and I'd love to connect on teams soon to check my understanding. My microsoft alias is the same as my github alias. Also, apologies if I missed some details on this somewhere, but you had a point in the original PR that partial packets were possible due to an "underlying incorrect assumption in the networking handling code". Can you share any additional details on this? It would help my understanding of how this change interacts with the networking code. Thanks! |
|
Email sent. After spending more time in debugging for this PR I think that the problem was an oversight caused by not understanding the original code completely. The async code was added a long time after the original sync code. The async code causes long delays when executing the replay mechanism used to support multi-packet payloads. Those delays mean that it's virtually impossible to observe partial packets while async. The async code takes entire input buffers and adds them to the packet list that it uses. It assumes that those input buffers are complete. This was a reasonable assumption given the observable data. Unfortunately I think it was wrong. If you run the code fast enough in async multi-packet situations you can encounter a network read where packet-size of data was requested but less than packet-size of data was available in the os buffer even though the complete packet will be packet-size in length. The read will return partial-first-size of data and the async code path does not check that partial-first-size against the size of the full packet defined in the packet header. This packet will be added to the async buffer list. I believe the current code is incapable of running fast enough to encounter the problem. When you speed up async multi-packet reads it occurs often enough to spoil test runs but it's not entirely deterministic and the more debugging logic you add to try and catch it the slower execution becomes and the less it happens. |
|
Thank you, that makes sense. I wanted to make sure there wasn't some additional low-level networking behavior we were working around. Email received; I'll reach out shortly. |
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.Windows.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs
Outdated
Show resolved
Hide resolved
...icrosoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObject.netcore.cs
Show resolved
Hide resolved
...icrosoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObject.netcore.cs
Show resolved
Hide resolved
| throw SQL.ParsingError(ParsingErrorState.ProcessSniPacketFailed); | ||
| } | ||
| } | ||
| return SNIPacketGetData(packet, _inBuff, ref dataSize); |
There was a problem hiding this comment.
Do you think it will be possible to avoid this function hop GetSniPacket->SNIPacketGetData?
There was a problem hiding this comment.
Possibly. It's part of the separation between the managed and unmanaged SNI implementations and looks like it's currently just a function to function indirection to the override. If it's really important I can collapse it but it's unlikely to cause any problems and it keeps the existing structure in place for those familiar with the layering making it easier to understand.
| @@ -393,60 +401,9 @@ private void ReadSniError(TdsParserStateObject stateObj, uint error) | |||
| AssertValidState(); | |||
| } | |||
|
|
|||
| public void ProcessSniPacket(PacketHandle packet, uint error) | |||
There was a problem hiding this comment.
Can we still keep the older function? I am still trying to understand all the checks that were removed with the new function call.
There was a problem hiding this comment.
The previous version of ProcessSniPacket ? Why would we? we can't swap out the implementations without also adding the ability to disable all partial packet checking. The core idea of the PR was that the change was limited to a small location, primarily ProcessSniPacket which is the communication point between two layers. Unfortunately the rather messy bug fixes that have been needed in other places make this more complex but ProcessSniPacket is still the primary location where new functionality is present.
There was a problem hiding this comment.
The need for multiplexing is clear given the current code structure, and the multiplexing logic itself is well contained. The tests give me confidence that we're multiplexing correctly.
I have less confidence in the overall orchestration of network calls and that the multiplexing logic is invoked correctly. There are too many different locations and situations in which the logic is invoked, I can't really reason about it. At the end of the day, we may just have to trust our existing code coverage.
Code Structure
The big thing that makes this challenging to reason about is that we're looping in two places (one for network reads, one for packet multiplexing), 4 or 5 layers apart and coordinating between them with state. The loops are also inverted from what you would expect (network read outside, multiplex inside). There's too much indirection at the moment. I really want us to move toward tighter loops at the same layer.
There's a lot of tricky stuff glossed over here, but conceptually I'm thinking something like:
SNIPacket GetPacket() {
EnsurePacket()
SNIPacket packet = ReadPacket()
ResetBuffer()
return packet
}
EnsurePacket()
{
if (buffer.length < HEADER_SIZE) {
EnsureBytes(HEADER_SIZE)
}
int data_size = buffer.readInt(...)
if (buffer.length < HEADER_SIZE + data_size) {
EnsureBytes(data_size)
}
}
EnsureBytes(int count, async)
{
while (buffer.length < count) {
NetworkRead(PACKET_SIZE)
}
}Taking some inspiration from npgsql: https://github.com/npgsql/npgsql/blob/main/src/Npgsql/Internal/NpgsqlConnector.cs#L1349
This setup inverts the current structure. In this way it's easier to see where network reads occur and orchestrate them properly to create a complete packet. You can get a complete packet out at the end and reference that rather than having future operations directly access the underlying buffer. It also removes the need for a lot of global state because the loops manage all of the state locally.
I want to explore any ways we can start moving towards this in this PR when we chat, but I won't block on it.
Performance numbers
It would be exciting to compile some performance numbers to show the impact of this change. Do you have any tests in mind that you used while implementing? I'll also think about anything I can add on.
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Packet.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| private static bool SetupRemainderPacket(Packet packet) |
There was a problem hiding this comment.
I'd really like to see the DataLength being set automatically inside the Packet class. As much as possible, the Packet class should enforce that it is internally consistent any time it is constructed or modified. This will help us make sure that we don't accidentally omit a setup call if we modify the multiplexing code in the future.
There was a problem hiding this comment.
The setter is used in 3 places and each is a different situation. In one case we know the value ahead of time, in the other two we don't know it and we're setting it using the function to fetch it from the header and in theory we can simply call the function for one of those but it'll do extra work that we don't need.
In general the Packet class is a dumb container of related data rather than being something that contains interesting logic for behaviours. Accessors has sanity checks but little more. Think of internals more like C than C#. Of course I can move behaviours into it but it increases cognitive load for little payback.
Excellent. The tests along with all the new issues that occurred with the linux CI runs are the best indicator that we're doing to get until it arrives with end users.
The multiplexing was invoked incorrectly. More precisely I'd missed a couple of places that pending reads could terminate and those have now been added. The effort required to untangle the internal layering is very high, the risk is very high and there are no immediate benefits to consumers. We've simplified and transformed some parts of the codebase over the years this repository has existed but the core parts are hard to change safely. In general large refactorings are unlikely to be helpful in this current codebase when the time could be spent on SqlClientX bringing it to feature parity.
There's also the smux layer. The multiplexer sits above that so I don't have to deal with untangling the smux header and the tds header.
I haven't run any performance numbers for this PR in isolation. In general I split my work into two categories; Perf and Fixes. For perf work numbers are the main driving force so I'll explain what I'm measuring and why. For fixes correctness is the goal and unless I have reason to believe that it will negatively affect performance and even then I'll make changes that slow things down because I consider correctness more important than speed. I consider this (and the previous part-1) a Fix pr for correctness. For this PR there will be some level of performance drop. I expect it to be small because on the happy path there should be few to no new allocations with some constant cpu required to evaluate the new path. On unhappy paths where we require reconstructions we will observe the query that encounters the fragmentation slow down when it hits the fragmented buffers and it will then continue seeing fragments until the query ends. The next query on the same connection will start with unfragmented packets again and will see the typical speed. If your workload is fixed length type heavy you should see little to no change. The small additional packet receive time will be hidden by the work required to get the values out of the packet. If your workload contains large multi-buffer values than this will slow you down more noticeably but part-3 of this series of PR's will linearize multi-buffer fetches so you're probably doing to see an overall speed increase. |
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Packet.cs
Outdated
Show resolved
Hide resolved
| int remainderLength = partialPacket.CurrentLength - partialPacket.RequiredLength; | ||
|
|
||
| partialPacket.CurrentLength -= remainderLength; |
There was a problem hiding this comment.
Can the partialPacket.CurrentLength be changed somewhere else asynchronously, because otherwise this is equivalent of writing partialPacket.CurrentLength = partialPacket.RequiredLength. Just trying to understand the reasoning here.
There was a problem hiding this comment.
It can't be changed elsewhere. I wrote it this way because it was logical to me, it documents what I'm doing not just what the result will be.
There was a problem hiding this comment.
I think this is a valid feedback, can you consider rewriting it to partialPacket.CurrentLength = partialPacket.RequiredLength or do you see any concerns?
There was a problem hiding this comment.
It's just a style difference as far as I can see. I feel that my version follows the flow of calculating and using whereas the alternative simply jumps to a resultant fact. It doesn't make enough difference for me to fight about it and with 2:1 votes I've changed it as suggested.
You may also note that I followed up the discussion about making partial packet disposal exception a debug only behaviour in 79f9fbc but forgot to mention it.
|
We discussed offline, and our concerns are about things going wrong in production for end users that could require us to patch the driver in future. Because this is an intricate change in the packet processing logic, we're considering enabling this feature by default, but be able to turn it off on demand using an App Context switch. The App Context switch would mean, we will preview this feature for couple of releases, till we have stabilized it and are confident the design is thorough and safe for permanent replacement, after which we can even remove the app context switch to remove old design. @Wraith2 One of our team members will get started on this, and collaborate with you to make that change in this PR, so we can have the App Context switch come in before the next PR you're working on. We're fully supportive of the changes, but also need to ensure we have a fallback plan for customers if anything goes wrong. |
|
Ok. I've made a start by adding the required appcontext switch, copying the current ProcessSni implementation into the muliplexer file as ProcessSniCompat and updating new tests to be conditional on the appcontext switch. The existing ProcessSni call will check the appcontext switch and call the old one if the compat switch is set. I'm not sure how you need to update the CI legs to account for testing both full sets to make sure we get coverage of both paths. |
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Packet.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Packet.cs
Outdated
Show resolved
Hide resolved
| /// <summary> | ||
| /// returns a boolean value indicating if the <see cref="DataLength"/> value has been set. | ||
| /// </summary> | ||
| public bool HasDataLength => _dataLength >= 0; |
There was a problem hiding this comment.
Consider != UnknownDataLength. That's the point of having a named constant, right?
There was a problem hiding this comment.
It's not quite the same thing. The constant is an arbitrary value which is known to produce a false result when this property is evaluated. The property defines ranges, the constant simply represents a single value inside that range that we use to have a specific named meaning. If you really want me to change it then I will but not using it is meaningful to me.
| { | ||
| if (error != 0) | ||
| { | ||
| if ((_parser.State == TdsParserState.Closed) || (_parser.State == TdsParserState.Broken)) |
There was a problem hiding this comment.
Consider _parser.State is TdsParserState.Closed or TdsParserState.Broken
There was a problem hiding this comment.
This falls into the category of maintaining consistent style. The change wouldn't improve functionality and would leave us with files in the solution using different syntax to do the same thing which can cause you not to be able to find them or have to mentally determine that both syntaxes mean the same thing.
If we're going to make this change I'd rather do it in a dedicated style update PR. I don't object to the new syntax I just think we should be consistent.
Split out from #2608 per discussion detailed in #2608 (comment)
Adds packet multiplexer and covering tests.