-
Notifications
You must be signed in to change notification settings - Fork 4.9k
SqlClient handle socket network error gracefully #36664
Changes from 1 commit
ace5fe4
81fe624
66dc458
e5fddbc
2190a0f
1d63519
268812e
87fdc75
7bc5200
c653c89
6ca1675
37867bd
090f99d
978d003
47d17e3
9924aa7
52206f1
7b320a0
5023c24
02d4a83
b181583
1cff273
168ac4c
663af00
e65153e
3f7071d
f9c9710
e3ec42f
b9bd354
7388539
f27d235
538cf16
4e66b9a
93564bf
cda6580
d8b0c41
59a1ed6
6c027ae
5bfb3d6
e9d1a10
9fb5661
b67328f
401436a
fdc1371
5b6a71f
cea252e
7ec7192
c9da58c
dd6984a
ffd0ae6
99943e1
3773daa
dc63fa0
1ae46a2
4672a7b
1c881ce
ef2ee12
e4dbfa4
0d9f64e
8c060b7
9e3ac32
8e84331
8067265
7b73456
d26c6cd
d6e36e4
af562e8
2cae2db
ce62a08
288ce93
137c4c1
31b6e6d
187330e
94f4a0d
8776610
d04c3c3
c6741f7
2cd5d08
599955a
6d519e5
fe5c12a
f843b9c
46f57de
6322a40
a4c96fb
f4b1514
741275c
a9b3eb6
ecd6a3e
8936954
541a7f5
2742172
5749844
12f5322
68b15a9
cc64aff
22382cd
3ff7e2c
3e6cefd
87e0533
4a75bd0
ba016fa
aaa33f8
8d5771a
7281061
a828645
6b4af6a
c986ec9
29bda78
6eeed98
c1730e5
00e9390
cd020c1
07f443b
cc01ec9
e2ab05d
c74f240
1d8cf4d
7345dd7
a7e2585
3262460
1a83ff9
f2a5dfa
2aa2518
8a22438
cce7870
09ed648
f5e2679
c3981ca
4c3d4a0
613400d
2062437
4fa6319
b06250e
e7fd166
3e2be47
5ae3c56
954abcd
52d4723
62c4161
ebbfe1b
f277935
1154e6c
1f07615
92e8eb4
a280216
0aa6ccc
822da31
b5a8985
68920bb
7336f57
f389a14
88b6ebe
9a2cd88
5928486
2f14413
81e0e2c
8c77bfb
36596d7
ff1ff5e
4d37ea9
25c1020
10d7d6f
679dba7
d601d2e
6bfaf41
03ae543
f78e988
4e3b46c
2041a42
1141460
ec652c1
2191763
071e698
339f951
66bcc2d
e2ecd2c
8d79b61
53f9e9e
7ed0c53
97b5d17
00279f8
c3c0efd
18e1fab
39cc915
0c9684c
ec1247f
e5d2b52
f80cf8c
f9f88bd
dbbabfb
18cd561
fa25b86
681eee7
1e3385e
fbc748b
3ac254f
bc8c222
818355a
d5a5f3c
dcd8090
60aee35
65f07a9
1824535
315fd00
1f9b84a
82eedbf
74dda69
c286ca0
bbb20b5
0fd3b7e
90d98b9
58048cc
9f21211
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ | |
|
|
||
| using System.Buffers; | ||
| using System.IO; | ||
| using System.Net.Sockets; | ||
| using System.Threading; | ||
| using System.Threading.Tasks; | ||
|
|
||
|
|
@@ -18,31 +19,39 @@ internal partial class SNIPacket | |
| /// <param name="callback">Completion callback</param> | ||
| public void ReadFromStreamAsync(Stream stream, SNIAsyncCallback callback) | ||
| { | ||
| // Treat local function as a static and pass all params otherwise as async will allocate | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you retain the comment?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can but it's no longer needed since a static can't refer to outer locals, sure you want to keep it? |
||
| async Task ReadFromStreamAsync(SNIPacket packet, SNIAsyncCallback cb, Task<int> task) | ||
| static async Task ReadFromStreamAsync(SNIPacket packet, SNIAsyncCallback cb, Task<int> task) | ||
| { | ||
| bool error = false; | ||
| uint errorCode = TdsEnums.SNI_SUCCESS; | ||
| try | ||
| { | ||
| packet._length = await task.ConfigureAwait(false); | ||
| if (packet._length == 0) | ||
| { | ||
| SNILoadHandle.SingletonInstance.LastError = new SNIError(SNIProviders.TCP_PROV, 0, SNICommon.ConnTerminatedError, string.Empty); | ||
| error = true; | ||
| errorCode = TdsEnums.SNI_WSAECONNRESET; | ||
| } | ||
| } | ||
| catch (IOException ioException) when ( | ||
| ioException?.InnerException is SocketException socketException && | ||
| socketException != null && | ||
| socketException.SocketErrorCode == SocketError.OperationAborted | ||
| ) | ||
| { | ||
| SNILoadHandle.SingletonInstance.LastError = new SNIError(SNIProviders.TCP_PROV, SNICommon.InternalExceptionError, socketException); | ||
| errorCode = TdsEnums.SNI_WSAECONNRESET; | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| SNILoadHandle.SingletonInstance.LastError = new SNIError(SNIProviders.TCP_PROV, SNICommon.InternalExceptionError, ex); | ||
| error = true; | ||
| errorCode = TdsEnums.SNI_ERROR; | ||
| } | ||
|
|
||
| if (error) | ||
| if (errorCode != TdsEnums.SNI_SUCCESS) | ||
| { | ||
| packet.Release(); | ||
| } | ||
|
|
||
| cb(packet, error ? TdsEnums.SNI_ERROR : TdsEnums.SNI_SUCCESS); | ||
| cb(packet, errorCode); | ||
| } | ||
|
|
||
| Task<int> t = stream.ReadAsync(_data, 0, _capacity, CancellationToken.None); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2691,7 +2691,7 @@ public void ReadAsyncCallback(IntPtr key, PacketHandle packet, uint error) | |
|
|
||
| // The mars physical connection can get a callback | ||
| // with a packet but no result after the connection is closed. | ||
| if (source == null && _parser._pMarsPhysicalConObj == this) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This condition removal doesn't seem right. This shouldn't be done in Native SNI. In native SNI, all callbacks happen on the same thread and this differentiation of the physical connection object is the only way to differentiate if the callback corresponds to this object. Also why is this correct for Managed SNI?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because I've introduced the close method on disconnection the physical connection will now be nulled, so checking if it's the current instance doesn't help. This catches the case where the socket is closed while the socket wait is in progress which causes a 0 packet receive. The socket in the native version is handled in the sni lib and the same condition doesn't occur there.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
What close method?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In SNIMarsHadle.HandleReceiveError, if (sniErrorCode == TdsEnums.SNI_WSAECONNRESET)
{
TdsParser parser = _callbackObject.Parser;
parser.State = TdsParserState.Broken;
parser.Connection.BreakConnection();
}BreakConnection dooms closes and does a ton of other stuff one of which is nulling the state object. Since the only two values that object can have are null and this and if it's either null or this and the source tcs is null it's not going to work the check on the state object seemed redundant.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The BreakConnection, only impacts breaking the connection for Managed SNI, this code would never run for Native SNI, and in that case the removal of this check can have unpredictable impact when Native SNI is being used.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. Then I'm going to have to put that back in and do some debugging and tracing to repo the problem it fixes and see if there's another way to detect it strictly in the managed side. I think if i'd seen one I would have used it so this might be tricky. |
||
| if (source == null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.