Modernize Stream.ReadEx #1306
Conversation
| } | ||
| } | ||
|
|
||
| #if !NETSTANDARD1X |
|
|
||
| return totalReadCount; | ||
| } | ||
| #else |
There was a problem hiding this comment.
Given that .NET Standard 1.x support was removed, the following code is dead code in my opinion.
There was a problem hiding this comment.
#1307 attempts to remove the remaining .NET Standard 1.x code
| //IO interruption not supported in this path. | ||
| currentReadCount = stream.Read(buffer, offset + totalReadCount, count - totalReadCount); | ||
| } | ||
| int currentReadCount = stream.ReadAsync(buffer, offset + totalReadCount, count - totalReadCount, cancellation).GetAwaiter().GetResult(); |
There was a problem hiding this comment.
https://learn.microsoft.com/en-us/dotnet/api/system.io.stream.readasync?view=net-10.0 mentions that this overload is support on:
| Technology | Versions |
|---|---|
| .NET Core | 1.0, 1.1, 2.0, 2.1, 2.2, 3.0, 3.1, 5, 6, 7, 8, 9, 10, 11 |
| .NET Framework | 4.5, 4.5.1, 4.5.2, 4.6, 4.6.1, 4.6.2, 4.7, 4.7.1, 4.7.2, 4.8, 4.8.1 |
| .NET Standard | 1.0, 1.1, 1.2, 1.3, 1.4, 1.5, 1.6, 2.0, 2.1 |
| UWP | 10.0 |
My understanding is that those are all supported platforms for NBitcoin. To be sure, I checked https://www.nuget.org/packages/NBitcoin#supportedframeworks-body-tab as well. I don't see anything missing.
btw: I considered stream.Read() method but it does not support cancellation so I use stream.ReadAsync().
|
This code saw some shit, so I would prefer not touching what is not broken. Why do you thing here is a source of unobserved exception? |
I found this PR WalletWasabi/WalletWasabi#8081 and #1097 which suggests so. However, that's all I know. |
|
I will try to dig more into it and if I find an actual proof, I will reopen the PR. |
2 participants
On Wasabi side, I can still see some unobserved exception. I suspect that
Stream.ReadExmight be another potential source of unobserved exceptions so I tried to modernize it.