Test and remove warnings of undefined behaviour #375
Conversation
It is no longer true that sending or receiving data from a closed socket will result in undefined behaviour. Version 3.0.0.0 defined this behaviour as an exception by introducing the `-1` sigil. This commit adds tests to ensure this behaviour remains stable and removes all documentation elluding to undefined behaviour.
tests/SimpleSpec.hs
Outdated
| @@ -111,6 +176,13 @@ spec = do | |||
|
|
|||
| client sock = send sock testMsg | |||
| tcpTest client server | |||
|
|
|||
| it "returns empty string at EOF" $ do | |||
There was a problem hiding this comment.
This is an additional test to cover the removal of EOF errors.
This commit breaks up the large `SimpleSpec` into its public module constituents. This provides a more granular and easier to manage overview of `network`'s test coverage. More test coverage has also been added to `Network.Socket.ByteString.Lazy` as it was mostly uncovered.
`unsafeInterleaveIO` means that closed socket failures are not thrown from `getContents`.
|
Though this PR looks excellent, I got this error on Mac (Mojave): |
|
At least, GHC 8.0.2 and GHC 8.6.3 got this error. |
|
This PR works well on Linux. |
|
Sounds like this found a Mac bug. We could pending the test for now and file an issue. |
|
|
|
My current opinion:
|
what operation? |
|
@llelf |
|
@kazu-yamamoto There is some discussion in that python thread about whether to document OS specific behaviour. They err on the side of not documenting it. Their reason being that opening the door to documenting OS specific quirks could lead to an unending list of quirks. I generally agree. My opinion would be:
|
|
@kazu-yamamoto Does this look good on your mojave machine? |
| sendAll sock lazyTestMsg `shouldThrow` anyException | ||
| tcpTest client server | ||
|
|
||
| #if !defined(darwin_HOST_OS) |
There was a problem hiding this comment.
For the record, this #ifdef is not needed. The solution is to fix a bug in getContents which does not handle I/O errors when calling shutdown(SHUT_RD) after EOF. While many platforms don't return errors from that call, some do. The getContents implementation needs to either drop the shutdown call or handle errors (perhaps just some I/O errors and not others, but see: https://ghc.haskell.org/trac/ghc/ticket/14730#comment:2)
There was a problem hiding this comment.
Likely also good to link to the original commit with the Shutdown added. Thanks for digging that up @vdukhovni
0d0b990#commitcomment-32203878
There was a problem hiding this comment.
I've added a catchall after shutdown, which I believe to be acceptable. @kazu-yamamoto and @vdukhovni I'm happy to hear reason why we should just remove shutdown instead.
Shutdown can fail on darwin. To avoid bugs in `getContents` we catch exceptions at shutdown and return content as if the shutdown was successful. Another strategy is to avoid calling `shutdown` all together, as this action is not required.
`onException` would also capture async errors and is too broad. Use `catchIOError`.
| @@ -66,7 +66,7 @@ getContents s = loop | |||
| sbs <- N.recv s defaultChunkSize | |||
| if S.null sbs | |||
| then do | |||
| shutdown s ShutdownReceive `onException` return () | |||
| shutdown s ShutdownReceive `catchIOError` const (return ()) | |||
There was a problem hiding this comment.
Yes, this is better, thanks! For example:
ghci> timeout 100000 $ catch (threadDelay 200000) (\e -> print (e :: SomeException))
<<timeout>>
Just ()
vs.
timeout 100000 $ catchIOError (threadDelay 200000) (\e -> print e)
Nothing
|
Merged. Thank you for your efforts! |
back port from PR haskell#375.
It is no longer true that sending or receiving data from a closed socket
will result in undefined behaviour. Version 3.0.0.0 defined this
behaviour as an exception by introducing the
-1sigil.This commit adds tests to ensure this behaviour remains stable and
removes all documentation elluding to undefined behaviour.