On 6/23/2011 3:29, Chris Muller wrote:
Just an FYI, the Magma test suite is pretty heavy on networking activity, but I didn't encounter any problems with this loaded.
But are you sure you were actually using it? I.e., if NetNameResolver>>useOldNetwork is true, you're not running *any* of that code.
So what needs to happen for us as a community to make a decision on whether to accept or reject this? I assume we will accept it, but maybe some of those more familiar with it can make some comments..? Who is our best networking expert who might want to review it and discuss?
I am assuming those are the IPv6 changes, right? The short version is this: The primitive side is fine, but there are some things I don't like about how IPv6 support is integrated into the image.
Let's start with name lookup. The DNS changes wrap everything into a SocketAddress structure which is somewhat reasonable except that it is platform specific (i.e., struct sockaddr) meaning that you'll be unable to transfer this across platforms and that you may be unable to even look at it after a restart (don't recall if it's invalidated upon image restart or platform change). Not having used it I don't know if that is going to be a problem or not but it seems very icky (and my guess is that it will be a problem for debugging, because when things go wrong you may want to look at a post-mortem image etc).
Another big issue in the name lookup is concurrency. When I looked at the Etoys code a while ago I noticed what looked like a serious lack of concurrency control in the name lookup. This was made even more problematic because (IIRC) there were some attributes that were queried against the last cached primitive lookup result (so initiating and completing a concurrent lookup might change the results of those calls). I don't know if that has been changed in the mean time, but concurrency in the name lookup code definitely needs some careful review.
Another issue I didn't like is the way name lookup tries to hide the difference between IPv4 and IPv6. If IPv6 were universally available this would be all right but given the state of adoption I think it's a mistake to try to hide the differences. I'd want to be able to specifically say that I'm looking for either IPv4, or IPv6 or both. And while (I think) I can do this using the new interface (NetNameResolver>>addressesForName: - IIRC, it looks both v4 and v6 addresses so one should be able to filter appropriately) there is currently no way to do this using the default interface (NetNameResolver>>addressForName:). This needs fixing.
The Socket code has fewer issues; mostly the new primitives are fine (and generally improvements on the previous primitive set). The biggest issue that I've seen is the awkward dependency on "NetNameResolver useOldNetwork" which is just plain unnecessary. I.e., instead of doing something like:
Socket>>connectNonBlockingTo: hostAddress port: port
"..."
NetNameResolver useOldNetwork ifTrue: [self primSocket: socketHandle connectTo: hostAddress port: port] ifFalse: [ hostAddress port: port. self connectNonBlockingTo: hostAddress]!
You can simply use:
hostAddress isSocketAddress ifTrue:[ hostAddress port: port. self connectNonBlockingTo: hostAddress]! ifFalse: [self primSocket: socketHandle connectTo: hostAddress port: port]
Why is the latter better? Simply because as a client I can decide if I want to use the IPv4-only version by passing in a ByteArray instead of a SocketAddress structure and can therefore decided on a per-instance basis whether I'd like to use IPv4-only or not.
Basically, the Socket prims should all just accept the IPv4-only 4-element byte array (if only for compatibility) and at that point it's up to the client to decide whether to use the IPv4-only interface, the IPv6-only interface or a mixed interface by either looking up or specifying the desired socket address / byte array.
Cheers, - Andreas