CancelIoEx and overal review of Win32-network #1627

coot · 2020-02-12T10:26:20Z

The primary goal of this PR was to add CancelIoEx to fix #1574. In doing so I changed the interface and the end result is that:

we don't use C type level wrappers for windows syscalls any more (apart from GetQueuedCompletionStatus where we need to use a windwo kernel C macro.
using GADTs to ensure that we use the right combination of OVERLAPPED / WSAOVERLAPPED, HANDLE / SOCKET and ErrCode / WsaErrCode.
there is a closed type family which ensures that some of the castPtr usage is actually safe, it is not exposed. It could be removed, but then using castPtr would leak out to various modules - so I preferred the more elegant and local, though type havier approach.
more tests, especially for error conditions in which we need to assure that we are not deallocating the stable pointer twice 💣

This PR is done on top of coot/windows-vectored-io (and it merges into that branch) - this makes the scope of this PR adequate for the proposed changes. For the moment the windows stuff is stacked on CI, but when devops will help with it I will be able to merge all the PRs in turn.

dcoutts

Comments so far, for first 2 patches.

Win32-network/src/System/Win32/Async/IOData.hsc

Win32-network/cbits/Win32Async.c

Win32-network/include/Win32-network.h

Win32-network/src/System/Win32/Async/File.hs

Win32-network/src/System/Win32/Async/IOData.hsc

Win32-network/src/System/Win32/Async/IOManager.hsc

coot · 2020-02-28T18:30:47Z

Closed by bors, it wasn't ment to be closed.

For Win32 >=2.7 import it directly.

Allocation and deallocation of 'IOData' associated with each async requests is done in Haskell. We use `System.Win32` api to allocate / deallocate from windows process heap (as we did in C). We can now use `ReadFile` and `WriteFile` syscalls without C wrappers. We are not using the ones from `Win32` package only beacuse we support Win32<2.7 which did not defined OVERLAPPED data type. This can be changed later on. 'IOData' data type is holds 'OVERLAPPED' passed to system calls and the stable pointer. As previously, this allows 'HsGetQueuedCompletionStatus' to use 'CONTAINING_RECORD' macro to recover it just from 'OVERLAPPED' member. 'withIODataPtr' is introduced which allocats 'IOData' on process heap. It takes most of responsibilities of 'waitForCompletion' function, which has been removed. This will allow us to add a 'CacnelIoEx' call in case the async I/O was interrupted by a GHC'c async exception.

If the 'IOManger' threads throws an 'IOException' and dies, there is no point on continuing running the application (any I/O would deadlock). The 'IOException' most likely signifies a bug in 'dequeueCompletionPackets'. The IOException is warpped in a newtype wrapper: 'IOManagerError'. This is all done to make it easier to debug (and test) problems that may arrise in the 'IOManager' thread.

If a thread which is blocked on reading or writing is cancelled with an 'AsyncException' use 'CancelIoEx' to cancel the IO action. When 'CancelIoEx' is used the 'IOManager' thread is notified and it will deallocate 'ioDataPtr' which was allocated on process heap. In such a case 'GetQueuedCompletionStatus' returns 'False' and sets 'ERROR_OPERATION_ABORTED' error.

- Add some haddocs to ErrCode - Fixed haddocs in 'connectnamedPipe'

Trigger conditions mentioned in MSDN `connectNamedPipe` doc https://docs.microsoft.com/en-us/windows/win32/api/namedpipeapi/nf-namedpipeapi-connectnamedpipe#remarks - ERROR_PIPE_CONNECTED - done - ERROR_NO_DATA - done - ERROR_PIPE_LISTENING - failed

* 'ERROR_INVALID_HANDLE' * 'ERROR_BROKEN_PIPE'

It takes ~20s - not good, but we can afford that.

WSAOVERLAPPED is used in winsock2 inplace of OVERLAPPED.

This is a perparatory work to support winsock2 io. Winsock2 is using WSAOVERLAPPED pointer unlike the rest of windows which is using OVERLAPPED. This patch provides type level distintion and various safe pointer casts. GADTs ensure that one will use the right overlapped struct / error codes (e.g. 'getLastError' vs 'wsaGetLastError') and sockets ('HANDLE' vs 'SOCKET'). Types also enforce that the `dequeueCompletionPackets` (IOManager thread) when dereferencing the shared stable pointer it will use the same type as the function that executes the async IO (e.g. 'readHandle', etc.). This is done by limiting unecessary polymorphism. Interestingly one can use 'DerivingVia' (combined with 'StandaloneDeriving') to derive instances for GADTs (see 'Overlapped' 'Storable' instances in 'System.Win32.Async.IOData').

This PR removes Win32Socket.c source file. We can just use the winsock2 functions through ffi directly without any C level wrappers.

* Excluding Win32-network library, PR #1627 fixes that.

This unitest (or experiment) checks how the `dequeueCompletionPackets` (IOMananger thread) behaves when closing a handle which is blocked on reading data. This either raises 'ResourceVanished` or `InvalidArgument` io exception in the thread that initiated the operation, but most importantly there is no exception in the IOManager thread.

The only safe call in Win32-network is 'GetQueuedCompletionStatus': it is the only blocking call; For that reason the library should be used with `-threaded` option, though this is not enforced in the `cabal` file.

dcoutts

This is looking very good. It is a lot simpler than before. Not too many remaining comments.

I see I had a pending stale review, so some of my early comments may be a bit out of date. If so, ignore them.

Win32-network/src/System/Win32/Async/IOManager.hsc

Win32-network/cbits/Win32Async.c

Win32-network/src/System/Win32/Async/IOManager.hsc

dcoutts · 2020-03-19T20:12:19Z

Win32-network/src/System/Win32/Async/Socket.hs

+        poke bufsPtr WSABuf {buf, len = fromIntegral size}
+        sendResult <- c_WSASend fd bufsPtr 1 nullPtr 0 lpOverlapped nullPtr
+        errorCode <- wsaGetLastError
+        if sendResult == 0 || errorCode == wSA_IO_PENDING


same Q as elsehwhere about whether this is the simplest condition.

I believe so. Tracing these values prints two cases (the first one is sendResult the other one is errorCode - 997, except being the police telephone number in Poland :D, is WSA_IO_PENDING) :

recvBuf -1, 997 recvBuf -1, 997 recvBuf -1, 997 recvBuf -1, 997 recvBuf 0, 0 recvBuf 0, 0 recvBuf -1, 997

^ this output was generated for recvBuf rather than sendBuf; for sendBuf it's similar (just 0, 0 is much more common):

sendBuf 0, 0 sendBuf 0, 0 sendBuf 0, 0 sendBuf 0, 0 sendBuf 0, 0 sendBuf -1, 997 sendBuf 0, 0 sendBuf 0, 0 sendBuf 0, 0 sendBuf 0, 0

Ok but in these examples the result being zero or non-zero always agrees with the error code. So we would be able to base our condition on either of them rather than both.

This is not a blocker, and I suggest we merge without addressing this now, but it certainly looks odd and we could clean this up later. File a low priority ticket.

Win32-network/src/System/Win32/Async/Socket.hs

* use intPtrToPtr * provide castOverlappedPtr - and document why it works

It is important that it is a tail recursive function. The analysis of results must be done inside `alloca` since as the result of this analysis we either can or cannot accesses `numberOfBytesPtr` (allocated by `alloca`).

Removed that the winio license applies to * ./cbits/Win32Async.c * ./src/System/Win32/Async.hs cbits were removed, and Async.hs file heavily rewritten.

dcoutts

Changes since last review looks great. Did we address everything? If so lets merge.

coot · 2020-03-24T14:33:57Z

bors r+

1627: CancelIoEx and overal review of Win32-network r=coot a=coot The primary goal of this PR was to add `CancelIoEx` to fix #1574. In doing so I changed the interface and the end result is that: * we don't use C type level wrappers for windows syscalls any more (apart from `GetQueuedCompletionStatus` where we need to use a windwo kernel C macro. * using `GADTs` to ensure that we use the right combination of `OVERLAPPED` / `WSAOVERLAPPED`, `HANDLE` / `SOCKET` and `ErrCode` / `WsaErrCode`. * there is a closed type family which ensures that some of the `castPtr` usage is actually safe, it is not exposed. It could be removed, but then using `castPtr` would leak out to various modules - so I preferred the more elegant and local, though type havier approach. * more tests, especially for error conditions in which we need to assure that we are not deallocating the stable pointer twice 💣 This PR is done on top of `coot/windows-vectored-io` (and it merges into that branch) - this makes the scope of this PR adequate for the proposed changes. For the moment the windows stuff is stacked on CI, but when devops will help with it I will be able to merge all the PRs in turn. 1840: Update dependency on cardano-ledger-specs r=mrBliss a=mrBliss Co-authored-by: Marcin Szamotulski <[email protected]> Co-authored-by: Thomas Winant <[email protected]>

iohk-bors · 2020-03-24T15:42:26Z

Build failed (retrying...)

buildkite/ouroboros-network

coot · 2020-03-24T16:40:22Z

bors: merge

iohk-bors · 2020-03-24T16:40:26Z

Already running a review

iohk-bors · 2020-03-24T17:47:45Z

Build succeeded

* Excluding Win32-network library, PR #1627 fixes that.

coot added networking Windows labels Feb 12, 2020

coot assigned dcoutts Feb 12, 2020

coot force-pushed the coot/windows-cancel-io branch from 964d6ab to 8cc6a81 Compare February 12, 2020 17:53

coot force-pushed the coot/windows-vectored-io branch from affb6ea to 3806a47 Compare February 14, 2020 18:30

coot force-pushed the coot/windows-cancel-io branch from 8cc6a81 to b22cd03 Compare February 14, 2020 19:12

coot force-pushed the coot/windows-vectored-io branch from 3806a47 to 9c20430 Compare February 18, 2020 16:35

coot force-pushed the coot/windows-cancel-io branch from b22cd03 to e6d8867 Compare February 18, 2020 16:36

dcoutts reviewed Feb 18, 2020

View reviewed changes

coot force-pushed the coot/windows-cancel-io branch from e6d8867 to 14cd1ac Compare February 19, 2020 10:54

coot force-pushed the coot/windows-vectored-io branch from 9c20430 to 085c24a Compare February 28, 2020 13:55

coot force-pushed the coot/windows-cancel-io branch from 67fcc3f to 935dfb3 Compare February 28, 2020 14:23

coot force-pushed the coot/windows-vectored-io branch from 085c24a to 7c582b7 Compare February 28, 2020 14:26

iohk-bors bot closed this Feb 28, 2020

coot reopened this Feb 28, 2020

coot changed the base branch from coot/windows-vectored-io to master February 28, 2020 18:31

coot force-pushed the coot/windows-cancel-io branch from 935dfb3 to cf8290a Compare February 28, 2020 18:31

coot added 12 commits March 16, 2020 10:52

Added OVERLAPPED data type

b46c2eb

For Win32 >=2.7 import it directly.

Win32-network - documentation improvments & fixes

57dbe1d

- Add some haddocs to ErrCode - Fixed haddocs in 'connectnamedPipe'

Added tests which trigger specific exceptions

669a0aa

* 'ERROR_INVALID_HANDLE' * 'ERROR_BROKEN_PIPE'

prop_PingPongPipelined: don't limit number of test cases

f6b7616

It takes ~20s - not good, but we can afford that.

Added WSAOVERLAPPED data and WSAIODATA struct.

ae98552

WSAOVERLAPPED is used in winsock2 inplace of OVERLAPPED.

Socket operations using withIOCPData

01a6bc2

This PR removes Win32Socket.c source file. We can just use the winsock2 functions through ffi directly without any C level wrappers.

Win32-network: do not expose internal modules

ab658d8

coot added this to the S9 2020-03-26 milestone Mar 16, 2020

coot added byron Required for a Byron mainnet: replace the old core nodes with cardano-node priority high labels Mar 16, 2020

coot force-pushed the coot/windows-cancel-io branch 2 times, most recently from 742b3be to 71f6279 Compare March 17, 2020 06:31

coot added a commit that referenced this pull request Mar 17, 2020

Enable most of the test

ba47db9

* Excluding Win32-network library, PR #1627 fixes that.

coot added a commit that referenced this pull request Mar 17, 2020

Enable most of the test

a3bbcec

* Excluding Win32-network library, PR #1627 fixes that.

coot force-pushed the coot/windows-cancel-io branch 2 times, most recently from 2cdb19a to 4fdac57 Compare March 17, 2020 15:41

coot added 3 commits March 17, 2020 16:43

Mark all non-blocking ffi as unsafe

aab8e9f

The only safe call in Win32-network is 'GetQueuedCompletionStatus': it is the only blocking call; For that reason the library should be used with `-threaded` option, though this is not enforced in the `cabal` file.

Added a simple README.md file

ec390db

coot force-pushed the coot/windows-cancel-io branch from 4fdac57 to ec390db Compare March 17, 2020 15:44

dcoutts requested changes Mar 19, 2020

View reviewed changes

coot added 4 commits March 23, 2020 20:16

address review comments

96d55cb

* use intPtrToPtr * provide castOverlappedPtr - and document why it works

Refactor dequeueCompletionPackets

5c7ef9b

It is important that it is a tail recursive function. The analysis of results must be done inside `alloca` since as the result of this analysis we either can or cannot accesses `numberOfBytesPtr` (allocated by `alloca`).

Use -rtsopts when compiling demo-chain-sync

899ca61

Updated NOTICE, added acknoledgment to README.md

e22a9bc

Removed that the winio license applies to * ./cbits/Win32Async.c * ./src/System/Win32/Async.hs cbits were removed, and Async.hs file heavily rewritten.

dcoutts approved these changes Mar 24, 2020

View reviewed changes

iohk-bors bot merged commit 5c253f7 into master Mar 24, 2020

iohk-bors bot deleted the coot/windows-cancel-io branch March 24, 2020 17:47

coot added a commit that referenced this pull request Mar 24, 2020

Enable most of the test

c8f430e

* Excluding Win32-network library, PR #1627 fixes that.

coot mentioned this pull request Mar 27, 2020

Test failure on Windows #1818

Closed

nfrisby mentioned this pull request Jan 12, 2021

Build consensus in GitHub actions #2806

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CancelIoEx and overal review of Win32-network #1627

CancelIoEx and overal review of Win32-network #1627

coot commented Feb 12, 2020

dcoutts left a comment

coot commented Feb 28, 2020

dcoutts left a comment

dcoutts Mar 19, 2020

coot Mar 24, 2020

coot Mar 24, 2020

dcoutts Mar 24, 2020

dcoutts left a comment

coot commented Mar 24, 2020

iohk-bors bot commented Mar 24, 2020

coot commented Mar 24, 2020

iohk-bors bot commented Mar 24, 2020

iohk-bors bot commented Mar 24, 2020

CancelIoEx and overal review of Win32-network #1627

CancelIoEx and overal review of Win32-network #1627

Conversation

coot commented Feb 12, 2020

dcoutts left a comment

Choose a reason for hiding this comment

coot commented Feb 28, 2020

dcoutts left a comment

Choose a reason for hiding this comment

dcoutts Mar 19, 2020

Choose a reason for hiding this comment

coot Mar 24, 2020

Choose a reason for hiding this comment

coot Mar 24, 2020

Choose a reason for hiding this comment

dcoutts Mar 24, 2020

Choose a reason for hiding this comment

dcoutts left a comment

Choose a reason for hiding this comment

coot commented Mar 24, 2020

iohk-bors bot commented Mar 24, 2020

Build failed (retrying...)

coot commented Mar 24, 2020

iohk-bors bot commented Mar 24, 2020

iohk-bors bot commented Mar 24, 2020

Build succeeded