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