Improving sendMsg and recvMsg #445

kazu-yamamoto · 2020-04-08T05:47:49Z

@vdukhovni Is this Proxy usage what you suggested?

@Mistuke Would you add commits to simplify the code in this PR?

vdukhovni · 2020-04-08T06:29:31Z

@vdukhovni Is this Proxy usage what you suggested?

Yes, that's what I had in mind. Do you agree that it is an improvement?

vdukhovni · 2020-04-08T07:09:12Z

@vdukhovni Is this Proxy usage what you suggested?

Yes, that's what I had in mind. Do you agree that it is an improvement?

But also the same change for the Windows version of the code.

vdukhovni · 2020-04-08T17:46:03Z

Network/Socket/Posix/Cmsg.hsc

@@ -87,24 +88,27 @@ filterCmsg cid cmsgs = filter (\cmsg -> cmsgId cmsg == cid) cmsgs
 --   Each control message type has a numeric 'CmsgId' and a 'Storable'
 --   data representation.
 class Storable a => ControlMessage a where
-    controlMessageId :: a -> CmsgId
+    controlMessageId :: Proxy a -> CmsgId


By the way, in similar situations I see other Haskell modules using a universally-quantified type variable here, instead of explicit Proxy. It makes no difference in practice. Not sure which is better style.

Suggested change

controlMessageId :: Proxy a -> CmsgId

controlMessageId :: p a -> CmsgId

Of course we still need a concrete phantom type (such as Proxy a) at the call sites.

You don't even need the Proxy argument if we drop the support for GHC 8.0 (cf. https://ryanglscott.github.io/2019/02/06/proxy-arguments-in-class-methods/). In general, polymorphic proxy type in class methods is not necessarily a good idea since it prevents GeneralisedNewtypeDeriving but I guess it doesn't matter here.

Did you mean "drop the support for GHC 7.x"?

I did mean GHC 8.0. GHC 8.0 doesn't support the following definition even with AllowAmbiguousTypes:

class C a where foo :: Int

I read the blog post, and it is quite interesting material, but I don't see a compelling need for newtype deriving with socket Control message structures, so we can go with the abstract proxy without fear, but the concrete Proxy is perhaps in fact less restrictive. The deriving issues don't come up, and a programmer passing undefined :: Proxy a rather than Proxy :: Proxy a deserves any resulting breakage.

So I'd keep it simple and stick with Proxy a and not yet require the fancier techniques that need much more recent advanced GHC features.

It does look like GHC 8.0 supports the simple signature with no argument. Running stack ghci --resolver lts-7.24 I get GHC 8.0.1, which handles the form with no arguments:

GHCi, version 8.0.1: http://www.haskell.org/ghc/ :? for help Prelude> :set -XTypeApplications Prelude> :set -XAllowAmbiguousTypes Prelude> class C a where { foo :: Int } Prelude> instance C Int where { foo = 0 } Prelude> instance C Char where { foo = 1 } Prelude> foo @Int 0 Prelude> foo @Char 1

But, it breaks GeneralizedNewtypeDeriving with GHC prior to 8.8. Do we care?
Here's the error with 8.6.5:

GHCi, version 8.6.5: http://www.haskell.org/ghc/ :? for help Prelude> :set -XTypeApplications Prelude> :set -XAllowAmbiguousTypes Prelude> :set -XGeneralizedNewtypeDeriving Prelude> class C a where { foo :: Int } Prelude> instance C Int where { foo = 0 } Prelude> instance C Char where { foo = 1 } Prelude> foo @Int 0 Prelude> foo @Char 1 Prelude> newtype Bar = Bar Int deriving(C) <interactive>:9:32: error: • Ambiguous type variable ‘a0’ arising from a use of ‘foo’ prevents the constraint ‘(C a0)’ from being solved. Probable fix: use a type annotation to specify what ‘a0’ should be. These potential instances exist: instance C Bar -- Defined at <interactive>:9:32 instance [safe] C Char -- Defined at <interactive>:6:10 instance [safe] C Int -- Defined at <interactive>:5:10 • In the third argument of ‘GHC.Prim.coerce’, namely ‘foo’ In the expression: GHC.Prim.coerce @Int @Int foo :: Int In an equation for ‘foo’: foo = GHC.Prim.coerce @Int @Int foo :: Int When typechecking the code for ‘foo’ in a derived instance for ‘C Bar’: To see the code I am typechecking, use -ddump-deriv

and success with 8.8.3:

GHCi, version 8.8.3: https://www.haskell.org/ghc/ :? for help Prelude> :set -XGeneralizedNewtypeDeriving Prelude> :set -XTypeApplications Prelude> :set -XAllowAmbiguousTypes Prelude> class C a where { foo :: Int } Prelude> instance C Int where { foo = 0 } Prelude> instance C Char where { foo = 1 } Prelude> foo @Int 0 Prelude> foo @Char 1 Prelude> newtype Bar = Bar Int deriving(C) Prelude> foo @Bar 0

OK. GHC 8.0 is supported. So, I prefer to controlMessageId :: CmsgId with AllowAmbiguousTypes and TypeApplications.

Do we really need deriving(ControlMessage)? I don't think so.

Works for me. I don't think that lack of support for newtype deriving in GHC 8.0–8.6 is a significant drawback, especially since Cmsg is a new interface with no legacy users.

vdukhovni · 2020-04-08T17:57:34Z

Network/Socket/Posix/Cmsg.hsc

 encodeCmsg x = unsafeDupablePerformIO $ do
    bs <- create siz $ \p0 -> do
        let p = castPtr p0
        poke p x
-    return $ Cmsg (controlMessageId x) bs
+    let cmsid = controlMessageId (Proxy :: Proxy a)


FWIW, and entirely cosmetic, with -XTypeApplications one could also, here and in decodeCmsg, write:

Suggested change

let cmsid = controlMessageId (Proxy :: Proxy a)

let cmsid = controlMessageId $ Proxy @a

TypeApplications was introduced in GHC 8.0.1. Since network v4 supports GHC 8 only, we can use it. But I don't know which is better.

It is a matter of style really, whichever you find more to your liking. There are more compelling uses of type applications, this one is not one of them, just cosmetic.

kazu-yamamoto · 2020-04-09T00:18:06Z

@Mistuke Please give a look at the CI results: https://ci.appveyor.com/project/eborden/network/builds/32030872

Can we get TTL on Windows?

kazu-yamamoto · 2020-04-09T00:19:04Z

@vdukhovni Yes, this is improvement! Thank you for your suggestion!

Mistuke · 2020-04-09T00:30:04Z

@Mistuke Please give a look at the CI results: https://ci.appveyor.com/project/eborden/network/builds/32030872

Can we get TTL on Windows?

We can, all the tests pass locally for me so it must be something with the AppVeyor network setup. I'll take a look tomorrow night along with simplifying the buffer code.

kazu-yamamoto · 2020-04-09T11:06:51Z

I pushed the commit to use AllowAmbiguousTypes and TypeApplications.
This is just for reference. I would withdraw it if necessary.

vdukhovni · 2020-04-09T17:57:21Z

I pushed the commit to use AllowAmbiguousTypes and TypeApplications.
This is just for reference. I would withdraw it if necessary.

Looks good.

kazu-yamamoto · 2020-04-10T20:33:39Z

@Mistuke Here is todo on Windows:

Fixing CI
Supporting the same change of this PR
Simplifying the code if necessary

Mistuke · 2020-04-19T17:55:28Z

I'm not really sure what's doing on with AppVeyor...

If I set IP_RECVTOS I get IP_TOS as expected back locally but on AppVeyor I get IP_TCLASS.

If I set IP_RECVTTL I get back locally IP_TTL but AppVeyor returns IP_RECVTTL.

Both of which are not according to https://docs.microsoft.com/en-us/windows/win32/winsock/ipproto-ip-socket-options So I wonder if they're not supported but getSocketOption is returning true for them...

Mistuke · 2020-04-26T04:49:01Z

Anyone got any thoughts?

vdukhovni · 2020-04-26T04:55:53Z

Anyone got any thoughts?

Sorry, I have minimal Windows development experience, and even less with AppVeyor. Rumour has it there's some way to debug AppVeyor interactively. I have no idea how.

kazu-yamamoto · 2020-04-29T23:10:33Z

@Mistuke Let's keep item 1 undone. Would you tackle item 2 and item 3?

Mistuke · 2020-05-03T08:46:29Z

@kazu-yamamoto yeah I will push those today

kazu-yamamoto · 2020-05-06T02:40:46Z

This PR has been merged. Thank you, guys, for your contributions!

kazu-yamamoto added 2 commits April 8, 2020 14:45

using Proxy.

1f5758d

fixing travis.

d3ff9b8

vdukhovni reviewed Apr 8, 2020

View reviewed changes

using AllowAmbiguousTypes and TypeApplications.

861e768

vdukhovni previously approved these changes Apr 10, 2020

View reviewed changes

This was referenced Apr 29, 2020

missing multicast support #381

Closed

Consider adding mcast support again? #446

Closed

Mistuke added 2 commits May 3, 2020 22:48

clean up recvMsgBuf

f8cb3e2

Win32: sync with linux changes

99e7f9a

kazu-yamamoto dismissed vdukhovni’s stale review via 99e7f9a May 6, 2020 02:34

kazu-yamamoto merged commit 1d61d74 into haskell:master May 6, 2020

kazu-yamamoto deleted the msg2 branch May 6, 2020 02:40

eborden mentioned this pull request May 17, 2020

Releasing version 3.1.2.0 #414

Closed

eborden added this to In progress in network 3.1.2.0 via automation May 20, 2020

eborden moved this from In progress to Done in network 3.1.2.0 May 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improving sendMsg and recvMsg #445

Improving sendMsg and recvMsg #445

kazu-yamamoto commented Apr 8, 2020

vdukhovni commented Apr 8, 2020

vdukhovni commented Apr 8, 2020

vdukhovni Apr 8, 2020 •

edited

Loading

fumieval Apr 9, 2020

kazu-yamamoto Apr 9, 2020

fumieval Apr 9, 2020

vdukhovni Apr 9, 2020

vdukhovni Apr 9, 2020

vdukhovni Apr 9, 2020

kazu-yamamoto Apr 9, 2020

kazu-yamamoto Apr 9, 2020

vdukhovni Apr 9, 2020

vdukhovni Apr 8, 2020

kazu-yamamoto Apr 9, 2020

vdukhovni Apr 9, 2020

kazu-yamamoto commented Apr 9, 2020

kazu-yamamoto commented Apr 9, 2020

Mistuke commented Apr 9, 2020

kazu-yamamoto commented Apr 9, 2020

vdukhovni commented Apr 9, 2020

kazu-yamamoto commented Apr 10, 2020 •

edited

Loading

Mistuke commented Apr 19, 2020

Mistuke commented Apr 26, 2020

vdukhovni commented Apr 26, 2020

kazu-yamamoto commented Apr 29, 2020

Mistuke commented May 3, 2020

kazu-yamamoto commented May 6, 2020

	controlMessageId :: Proxy a -> CmsgId
	controlMessageId :: p a -> CmsgId

	let cmsid = controlMessageId (Proxy :: Proxy a)
	let cmsid = controlMessageId $ Proxy @a

Improving sendMsg and recvMsg #445

Improving sendMsg and recvMsg #445

Conversation

kazu-yamamoto commented Apr 8, 2020

vdukhovni commented Apr 8, 2020

vdukhovni commented Apr 8, 2020

vdukhovni Apr 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kazu-yamamoto commented Apr 9, 2020

kazu-yamamoto commented Apr 9, 2020

Mistuke commented Apr 9, 2020

kazu-yamamoto commented Apr 9, 2020

vdukhovni commented Apr 9, 2020

kazu-yamamoto commented Apr 10, 2020 • edited Loading

Mistuke commented Apr 19, 2020

Mistuke commented Apr 26, 2020

vdukhovni commented Apr 26, 2020

kazu-yamamoto commented Apr 29, 2020

Mistuke commented May 3, 2020

kazu-yamamoto commented May 6, 2020

vdukhovni Apr 8, 2020 •

edited

Loading

kazu-yamamoto commented Apr 10, 2020 •

edited

Loading