WebClient test fails in trunk

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

WebClient test fails in trunk

Frank Shearar-3
Hi,

I took WebClient for a spin in a trunk image, and found a failing
test: WebClientServerTest >> #testServerDestroy throws a
"ConnectionClosed: Connection closed while waiting for data". That
sounds familiar - I think this was introduced in a recent Network
change.

Still, it means WebClient needs fixing. I'll try fix it on my evening
commute, but if anyone feels like racing me, I'll be happy!

frank

Reply | Threaded
Open this post in threaded view
|

Re: WebClient test fails in trunk

Levente Uzonyi-2
On Tue, 3 Jul 2012, Frank Shearar wrote:

> Hi,
>
> I took WebClient for a spin in a trunk image, and found a failing
> test: WebClientServerTest >> #testServerDestroy throws a
> "ConnectionClosed: Connection closed while waiting for data". That
> sounds familiar - I think this was introduced in a recent Network
> change.
>
> Still, it means WebClient needs fixing. I'll try fix it on my evening
> commute, but if anyone feels like racing me, I'll be happy!

Interesting, I had no issue with it on windows, except for some extension
methods trying to use the CrLf class variable in HTTPSocket (or some other
class), but that variable doesn't exist.


Levente

>
> frank
>
>

Reply | Threaded
Open this post in threaded view
|

Re: WebClient test fails in trunk

Frank Shearar-3
On 3 July 2012 17:38, Levente Uzonyi <[hidden email]> wrote:

> On Tue, 3 Jul 2012, Frank Shearar wrote:
>
>> Hi,
>>
>> I took WebClient for a spin in a trunk image, and found a failing
>> test: WebClientServerTest >> #testServerDestroy throws a
>> "ConnectionClosed: Connection closed while waiting for data". That
>> sounds familiar - I think this was introduced in a recent Network
>> change.
>>
>> Still, it means WebClient needs fixing. I'll try fix it on my evening
>> commute, but if anyone feels like racing me, I'll be happy!
>
>
> Interesting, I had no issue with it on windows, except for some extension
> methods trying to use the CrLf class variable in HTTPSocket (or some other
> class), but that variable doesn't exist.

Which reminds me of an important fact I should mention: I'm testing
this on an Ubuntu Lynx box.

frank

> Levente
>
>>
>> frank
>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: WebClient test fails in trunk

Frank Shearar-3
On 3 July 2012 17:42, Frank Shearar <[hidden email]> wrote:

> On 3 July 2012 17:38, Levente Uzonyi <[hidden email]> wrote:
>> On Tue, 3 Jul 2012, Frank Shearar wrote:
>>
>>> Hi,
>>>
>>> I took WebClient for a spin in a trunk image, and found a failing
>>> test: WebClientServerTest >> #testServerDestroy throws a
>>> "ConnectionClosed: Connection closed while waiting for data". That
>>> sounds familiar - I think this was introduced in a recent Network
>>> change.
>>>
>>> Still, it means WebClient needs fixing. I'll try fix it on my evening
>>> commute, but if anyone feels like racing me, I'll be happy!
>>
>>
>> Interesting, I had no issue with it on windows, except for some extension
>> methods trying to use the CrLf class variable in HTTPSocket (or some other
>> class), but that variable doesn't exist.
>
> Which reminds me of an important fact I should mention: I'm testing
> this on an Ubuntu Lynx box.

Peek tries to read data, which waits for data, which raises the
ConnectionClosed. If peek must return nil when the socket's closed,
I'd expect to see the receiveData wrapped appropriately. Something
like

peek
        "Return next byte, if inBuffer is empty
        we recieve some more data and try again.
        Do not consume the byte."

        self atEnd ifTrue: [^nil].
        self isInBufferEmpty ifTrue:
                [[self receiveData] on: ConnectionClosed do: [^nil] "<--"
                self atEnd ifTrue: [^nil]].
        ^inBuffer at: lastRead+1

That's a random hack that makes the test pass (and doesn't break any
others); I don't know if it has any negative consequences elsewhere. I
guess the method as above means "return the next thing in the stream,
unless we're at the end of the stream, or there's no data", which
doesn't seem entirely unreasonable.

frank

> frank
>
>> Levente
>>
>>>
>>> frank
>>>
>>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: WebClient test fails in trunk

Levente Uzonyi-2
On Tue, 3 Jul 2012, Frank Shearar wrote:

> On 3 July 2012 17:42, Frank Shearar <[hidden email]> wrote:
>> On 3 July 2012 17:38, Levente Uzonyi <[hidden email]> wrote:
>>> On Tue, 3 Jul 2012, Frank Shearar wrote:
>>>
>>>> Hi,
>>>>
>>>> I took WebClient for a spin in a trunk image, and found a failing
>>>> test: WebClientServerTest >> #testServerDestroy throws a
>>>> "ConnectionClosed: Connection closed while waiting for data". That
>>>> sounds familiar - I think this was introduced in a recent Network
>>>> change.
>>>>
>>>> Still, it means WebClient needs fixing. I'll try fix it on my evening
>>>> commute, but if anyone feels like racing me, I'll be happy!
>>>
>>>
>>> Interesting, I had no issue with it on windows, except for some extension
>>> methods trying to use the CrLf class variable in HTTPSocket (or some other
>>> class), but that variable doesn't exist.
>>
>> Which reminds me of an important fact I should mention: I'm testing
>> this on an Ubuntu Lynx box.

The VM also matters in this case. I tried it with an older CogVM which
doesn't have the new socket primitives.

>
> Peek tries to read data, which waits for data, which raises the
> ConnectionClosed. If peek must return nil when the socket's closed,
> I'd expect to see the receiveData wrapped appropriately. Something
> like
>
> peek
> "Return next byte, if inBuffer is empty
> we recieve some more data and try again.
> Do not consume the byte."
>
> self atEnd ifTrue: [^nil].
> self isInBufferEmpty ifTrue:
> [[self receiveData] on: ConnectionClosed do: [^nil] "<--"
> self atEnd ifTrue: [^nil]].
> ^inBuffer at: lastRead+1
>
> That's a random hack that makes the test pass (and doesn't break any
> others); I don't know if it has any negative consequences elsewhere. I
> guess the method as above means "return the next thing in the stream,
> unless we're at the end of the stream, or there's no data", which
> doesn't seem entirely unreasonable.

I never really used or looked deeply into SocketStream, so I'm not sure if
it's okay to catch that exception.


Levente

>
> frank
>
>> frank
>>
>>> Levente
>>>
>>>>
>>>> frank
>>>>
>>>>
>>>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: WebClient test fails in trunk

Frank Shearar-3
On 4 July 2012 22:46, Levente Uzonyi <[hidden email]> wrote:

> On Tue, 3 Jul 2012, Frank Shearar wrote:
>
>> On 3 July 2012 17:42, Frank Shearar <[hidden email]> wrote:
>>>
>>> On 3 July 2012 17:38, Levente Uzonyi <[hidden email]> wrote:
>>>>
>>>> On Tue, 3 Jul 2012, Frank Shearar wrote:
>>>>
>>>>> Hi,
>>>>>
>>>>> I took WebClient for a spin in a trunk image, and found a failing
>>>>> test: WebClientServerTest >> #testServerDestroy throws a
>>>>> "ConnectionClosed: Connection closed while waiting for data". That
>>>>> sounds familiar - I think this was introduced in a recent Network
>>>>> change.
>>>>>
>>>>> Still, it means WebClient needs fixing. I'll try fix it on my evening
>>>>> commute, but if anyone feels like racing me, I'll be happy!
>>>>
>>>>
>>>>
>>>> Interesting, I had no issue with it on windows, except for some
>>>> extension
>>>> methods trying to use the CrLf class variable in HTTPSocket (or some
>>>> other
>>>> class), but that variable doesn't exist.
>>>
>>>
>>> Which reminds me of an important fact I should mention: I'm testing
>>> this on an Ubuntu Lynx box.
>
>
> The VM also matters in this case. I tried it with an older CogVM which
> doesn't have the new socket primitives.
>
>
>>
>> Peek tries to read data, which waits for data, which raises the
>> ConnectionClosed. If peek must return nil when the socket's closed,
>> I'd expect to see the receiveData wrapped appropriately. Something
>> like
>>
>> peek
>>         "Return next byte, if inBuffer is empty
>>         we recieve some more data and try again.
>>         Do not consume the byte."
>>
>>         self atEnd ifTrue: [^nil].
>>         self isInBufferEmpty ifTrue:
>>                 [[self receiveData] on: ConnectionClosed do: [^nil] "<--"
>>                 self atEnd ifTrue: [^nil]].
>>         ^inBuffer at: lastRead+1
>>
>> That's a random hack that makes the test pass (and doesn't break any
>> others); I don't know if it has any negative consequences elsewhere. I
>> guess the method as above means "return the next thing in the stream,
>> unless we're at the end of the stream, or there's no data", which
>> doesn't seem entirely unreasonable.
>
>
> I never really used or looked deeply into SocketStream, so I'm not sure if
> it's okay to catch that exception.

Indeed! I should add that while it's a WebClient test failing, the
above is in the Network package. The WebClient test basically asks
"what do we peek after a socket's been destroyed?" and expects the
answer to be nil. If I understand the Stream protocol correctly (which
I almost certainly don't), returning nil would seem to be the
appropriate return value when you're at the end of the stream. In
Xtreams the answer would be "you get an Incomplete exception".

frank

> Levente
>
>>
>> frank
>>
>>> frank
>>>
>>>> Levente
>>>>
>>>>>
>>>>> frank
>>>>>
>>>>>
>>>>
>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: WebClient test fails in trunk

David T. Lewis
In reply to this post by Levente Uzonyi-2
On Wed, Jul 04, 2012 at 11:46:58PM +0200, Levente Uzonyi wrote:

> On Tue, 3 Jul 2012, Frank Shearar wrote:
>
> >On 3 July 2012 17:42, Frank Shearar <[hidden email]> wrote:
> >>On 3 July 2012 17:38, Levente Uzonyi <[hidden email]> wrote:
> >>>On Tue, 3 Jul 2012, Frank Shearar wrote:
> >>>
> >>>>Hi,
> >>>>
> >>>>I took WebClient for a spin in a trunk image, and found a failing
> >>>>test: WebClientServerTest >> #testServerDestroy throws a
> >>>>"ConnectionClosed: Connection closed while waiting for data". That
> >>>>sounds familiar - I think this was introduced in a recent Network
> >>>>change.
> >>>>
> >>>>Still, it means WebClient needs fixing. I'll try fix it on my evening
> >>>>commute, but if anyone feels like racing me, I'll be happy!
> >>>
> >>>
> >>>Interesting, I had no issue with it on windows, except for some extension
> >>>methods trying to use the CrLf class variable in HTTPSocket (or some
> >>>other
> >>>class), but that variable doesn't exist.
> >>
> >>Which reminds me of an important fact I should mention: I'm testing
> >>this on an Ubuntu Lynx box.
>
> The VM also matters in this case. I tried it with an older CogVM which
> doesn't have the new socket primitives.
I wanted to check that the recent network updates were not causing
problems for WebClient. I found that some updates to WebClient are
needed to account for the fact that network addresses are now being
represented as SocketAddress rather than ByteArray when running on
a VM that supports the new network primitives.

Attached is a change set that addresses all the issues that I could
find. I'm assuming that the testServerDestroy issue discussed above
has already been addressed, as I had no failures for that test.

>From the preamble:

  Change Set: WebClient-networkPatches-dtl
  Date: 15 July 2012
  Author: David T Lewis
 
  These patches update WebClient to work with new network code in Squeak
  trunk (all tests green).
 
  Explanation: When using the new network code along with a VM that supports the
  IPV6 primitives, socket addresses are instances of SocketAddress rather than
  ByteArray. Conversion of a socket address to a string must be implemented
  differently for SocketAddress.
 
  Also use String crlf instead of undeclared CrLf in HTTPSocket class>>httpPostMultipart:args:accept:request:
 
  Note: Squeak automatically uses SocketAddress if the VM supports it at image
  start time. To force use of old style ByteArray addresses, evaluate
  NetNameResolver useOldNetwork: true.
 
  Follow up: The WebClient methods in HTTPSocket are marked as overrides (category
  *WebClient-HTTP-override) but are not overrides with respect to Squeak trunk.
  The override category prevents these methods from appearing in a change set
  fileout. Should these methods just be adopted in trunk?

Dave




WebClient-networkPatches-dtl.1.cs.gz (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: WebClient test fails in trunk

Andreas.Raab
Hi David -

Are these patches needed only for IPv6 or also to support IPv4? If the latter, this would bother me greatly. I can understand why IPv6 may be implemented in incompatible ways but I can't see why there should be an incompatible interface to IPv4 addresses which have been used for eternity in Squeak. In particular given that (I believe) SocketAddress is a subclass of ByteArray it strikes me as not particularly hard to have Sockets support ByteArrays of length 4 as IPv4 socket addresses (and perhaps return instances of SocketAddress with length 4 or so). In any case, I'm not looking forward to seeing every last bit of networking code break - there has to be a better way...

Cheers,
  - Andreas

-------- Original-Nachricht --------
Datum: Sun, 15 Jul 2012 11:46:54 -0400
Von: "David T. Lewis" <[hidden email]>
An: The general-purpose Squeak developers list <[hidden email]>, [hidden email]
Betreff: Re: [squeak-dev] WebClient test fails in trunk

On Wed, Jul 04, 2012 at 11:46:58PM +0200, Levente Uzonyi wrote:

> On Tue, 3 Jul 2012, Frank Shearar wrote:
>
> >On 3 July 2012 17:42, Frank Shearar <[hidden email]> wrote:
> >>On 3 July 2012 17:38, Levente Uzonyi <[hidden email]> wrote:
> >>>On Tue, 3 Jul 2012, Frank Shearar wrote:
> >>>
> >>>>Hi,
> >>>>
> >>>>I took WebClient for a spin in a trunk image, and found a failing
> >>>>test: WebClientServerTest >> #testServerDestroy throws a
> >>>>"ConnectionClosed: Connection closed while waiting for data". That
> >>>>sounds familiar - I think this was introduced in a recent Network
> >>>>change.
> >>>>
> >>>>Still, it means WebClient needs fixing. I'll try fix it on my evening
> >>>>commute, but if anyone feels like racing me, I'll be happy!
> >>>
> >>>
> >>>Interesting, I had no issue with it on windows, except for some extension
> >>>methods trying to use the CrLf class variable in HTTPSocket (or some
> >>>other
> >>>class), but that variable doesn't exist.
> >>
> >>Which reminds me of an important fact I should mention: I'm testing
> >>this on an Ubuntu Lynx box.
>
> The VM also matters in this case. I tried it with an older CogVM which
> doesn't have the new socket primitives.

I wanted to check that the recent network updates were not causing
problems for WebClient. I found that some updates to WebClient are
needed to account for the fact that network addresses are now being
represented as SocketAddress rather than ByteArray when running on
a VM that supports the new network primitives.

Attached is a change set that addresses all the issues that I could
find. I'm assuming that the testServerDestroy issue discussed above
has already been addressed, as I had no failures for that test.

From the preamble:

Change Set: WebClient-networkPatches-dtl
Date: 15 July 2012
Author: David T Lewis

These patches update WebClient to work with new network code in Squeak
trunk (all tests green).

Explanation: When using the new network code along with a VM that supports the
IPV6 primitives, socket addresses are instances of SocketAddress rather than
ByteArray. Conversion of a socket address to a string must be implemented
differently for SocketAddress.

Also use String crlf instead of undeclared CrLf in HTTPSocket class>>httpPostMultipart:args:accept:request:

Note: Squeak automatically uses SocketAddress if the VM supports it at image
start time. To force use of old style ByteArray addresses, evaluate
NetNameResolver useOldNetwork: true.

Follow up: The WebClient methods in HTTPSocket are marked as overrides (category
*WebClient-HTTP-override) but are not overrides with respect to Squeak trunk.
The override category prevents these methods from appearing in a change set
fileout. Should these methods just be adopted in trunk?

Dave



Reply | Threaded
Open this post in threaded view
|

Re: WebClient test fails in trunk

David T. Lewis
Hi Andreas,

The changes should hopefully work with any image regardless of whether
the new socket code is present, and regardless of what flavor of VM and
SocketPlugin are running.

There should be no compatibity issues either for IPv4 or IPv6. The only
problem was that if the four-byte socket address is being represented by
a SocketAddress (subclass of ByteArray) rather than a ByteArray, then
converting it to a host name string with this does not work:

  String streamContents:[:s| anAddress
    do:[:b| s print: b] separatedBy:[s nextPut: $.]]

Aside from that, no real issues as far as I know (other than the trivial
undeclared CrLf issue).

A SocketAddress is a kind of ByteArray, but it represents an opaque
reference to a data structure managed in the plugin. That data structure
does include the four-byte network address (for IPv4) but this should
not be directly referenced from the image.

For example, on my PC with a 64 bit VM, the local address 127.0.0.2
is represented by a SocketAddress that, if printed out byte-by-byte,
would look like this:

 '117.49.82.81.16.0.0.0.2.0.0.0.127.0.0.2.0.0.0.0.0.0.0.0'

This is an 8 byte pointer, followed by the 32 bit network address, followed
by some other stuff I don't remember. But the printString for this object
provides a more useful representation:

 '127.0.0.2(linux-jh8m),0(0)'

Thus for a network address represented in the traditional way, printing
it out as bytes separated by $. does the right thing, and for the same
address represented by a SocketAddress, the first portion of the printString
up to $( is suitable. I did not check to see if this works with an IPv6
address, but it's good enough to cover the cases in the WebClient tests.

I am guessing that for logging purposes, you would want to use the complete
SocketAddress>>printString '127.0.0.2(linux-jh8m),0(0)' while for converting
a socket address to a host string you would want only the address portion
'127.0.0.2', so I put two separate methods in WebUtils for those two cases.

Dave

On Sun, Jul 15, 2012 at 06:48:15PM +0200, Andreas Raab wrote:

> Hi David -
>  
>  Are these patches needed only for IPv6 or also to support IPv4? If the  
> latter, this would bother me greatly. I can understand why IPv6 may be  
> implemented in incompatible ways but I can't see why there should be an  
> incompatible interface to IPv4 addresses which have been used for  eternity
> in Squeak. In particular given that (I believe) SocketAddress is a subclass
> of ByteArray it strikes me as not particularly hard to have Sockets support
> ByteArrays of length 4 as IPv4 socket addresses (and perhaps return
> instances of SocketAddress with length 4 or so). In any case, I'm not
> looking forward to seeing every last bit of networking code break - there
> has to be a better way...
>
>  Cheers,
>  ?? - Andreas
>
>  
> >            
> > -------- Original-Nachricht --------
> > Datum: Sun, 15 Jul 2012 11:46:54 -0400
> > Von: "David T. Lewis" <[hidden email]>
> > An: The general-purpose Squeak developers list
> > <[hidden email]>, [hidden email]
> > Betreff: Re: [squeak-dev] WebClient test fails in trunk
> >
> >             On Wed, Jul 04, 2012 at 11:46:58PM +0200, Levente Uzonyi
> > wrote:
> > > On Tue, 3 Jul 2012, Frank Shearar wrote:
> > >
> > > >On 3 July 2012 17:42, Frank Shearar <[hidden email]> wrote:
> > > >>On 3 July 2012 17:38, Levente Uzonyi <[hidden email]> wrote:
> > > >>>On Tue, 3 Jul 2012, Frank Shearar wrote:
> > > >>>
> > > >>>>Hi,
> > > >>>>
> > > >>>>I took WebClient for a spin in a trunk image, and found a failing
> > > >>>>test: WebClientServerTest >> #testServerDestroy throws a
> > > >>>>"ConnectionClosed: Connection closed while waiting for data". That
> > > >>>>sounds familiar - I think this was introduced in a recent Network
> > > >>>>change.
> > > >>>>
> > > >>>>Still, it means WebClient needs fixing. I'll try fix it on my
> > evening
> > > >>>>commute, but if anyone feels like racing me, I'll be happy!
> > > >>>
> > > >>>
> > > >>>Interesting, I had no issue with it on windows, except for some
> > extension
> > > >>>methods trying to use the CrLf class variable in HTTPSocket (or some
> > > >>>other
> > > >>>class), but that variable doesn't exist.
> > > >>
> > > >>Which reminds me of an important fact I should mention: I'm testing
> > > >>this on an Ubuntu Lynx box.
> > >
> > > The VM also matters in this case. I tried it with an older CogVM which
> > > doesn't have the new socket primitives.
> >
> > I wanted to check that the recent network updates were not causing
> > problems for WebClient. I found that some updates to WebClient are
> > needed to account for the fact that network addresses are now being
> > represented as SocketAddress rather than ByteArray when running on
> > a VM that supports the new network primitives.
> >
> > Attached is a change set that addresses all the issues that I could
> > find. I'm assuming that the testServerDestroy issue discussed above
> > has already been addressed, as I had no failures for that test.
> >
> > From the preamble:
> >
> >   Change Set: WebClient-networkPatches-dtl
> >   Date: 15 July 2012
> >   Author: David T Lewis
> >  
> >   These patches update WebClient to work with new network code in Squeak
> >   trunk (all tests green).
> >  
> >   Explanation: When using the new network code along with a VM that
> > supports the
> >   IPV6 primitives, socket addresses are instances of SocketAddress rather
> > than
> >   ByteArray. Conversion of a socket address to a string must be
> > implemented
> >   differently for SocketAddress.
> >  
> >   Also use String crlf instead of undeclared CrLf in HTTPSocket
> > class>>httpPostMultipart:args:accept:request:
> >  
> >   Note: Squeak automatically uses SocketAddress if the VM supports it at
> > image
> >   start time. To force use of old style ByteArray addresses, evaluate
> >   NetNameResolver useOldNetwork: true.
> >  
> >   Follow up: The WebClient methods in HTTPSocket are marked as overrides
> > (category
> >   *WebClient-HTTP-override) but are not overrides with respect to Squeak
> > trunk.
> >   The override category prevents these methods from appearing in a change
> > set
> >   fileout. Should these methods just be adopted in trunk?
> >
> > Dave
> >
> >
>        

Reply | Threaded
Open this post in threaded view
|

Re: WebClient test fails in trunk

Andreas.Raab
Hi David -

The old IPv6 code. Sigh. Unfortunately, this code was written with the idea of being a complete rewrite and not an add-on to the existing library but it's now used as an add-on and this causes all sorts of problems, including major compatibility issues.

I really do think that code needs serious overhaul. It basically needs to be rewritten such that:
a) The external interface remains simply a byte array (be that an IPv4 style 4-element byte array or some sort of SocketAddress which really *is* a socket address and not some plugin-internal structure combining address, port, family, and the phase of the moon)
b) SocketAddress[Info] is used privately within the socket and never exposed to a client unless that client specifically asks for it (but definitely not via stuff like Socket>>[local|remote]Address)
c) NetNameResolver needs to return IPv4 addresses by default and only answer IPv6 if the client specifically requests it.

Without this you're basically playing whack-a-mole trying to catch bugs that pop up randomly. A straightforward user like WebClient should be able to use Sockets without any change, defaulting to IPv4, and only when it specifically requests IPv6 support should it be exposed to IPv6. Otherwise this will be a complete nightmare.

Cheers,
  - Andreas

-------- Original-Nachricht --------
Datum: Sun, 15 Jul 2012 14:53:59 -0400
Von: "David T. Lewis" <[hidden email]>
An: Andreas Raab <[hidden email]>
CC: [hidden email]
Betreff: Re: [squeak-dev] WebClient test fails in trunk

Hi Andreas,

The changes should hopefully work with any image regardless of whether
the new socket code is present, and regardless of what flavor of VM and
SocketPlugin are running.

There should be no compatibity issues either for IPv4 or IPv6. The only
problem was that if the four-byte socket address is being represented by
a SocketAddress (subclass of ByteArray) rather than a ByteArray, then
converting it to a host name string with this does not work:

String streamContents:[:s| anAddress
do:[:b| s print: b] separatedBy:[s nextPut: $.]]

Aside from that, no real issues as far as I know (other than the trivial
undeclared CrLf issue).

A SocketAddress is a kind of ByteArray, but it represents an opaque
reference to a data structure managed in the plugin. That data structure
does include the four-byte network address (for IPv4) but this should
not be directly referenced from the image.

For example, on my PC with a 64 bit VM, the local address 127.0.0.2
is represented by a SocketAddress that, if printed out byte-by-byte,
would look like this:

'117.49.82.81.16.0.0.0.2.0.0.0.127.0.0.2.0.0.0.0.0.0.0.0'

This is an 8 byte pointer, followed by the 32 bit network address, followed
by some other stuff I don't remember. But the printString for this object
provides a more useful representation:

'127.0.0.2(linux-jh8m),0(0)'

Thus for a network address represented in the traditional way, printing
it out as bytes separated by $. does the right thing, and for the same
address represented by a SocketAddress, the first portion of the printString
up to $( is suitable. I did not check to see if this works with an IPv6
address, but it's good enough to cover the cases in the WebClient tests.

I am guessing that for logging purposes, you would want to use the complete
SocketAddress>>printString '127.0.0.2(linux-jh8m),0(0)' while for converting
a socket address to a host string you would want only the address portion
'127.0.0.2', so I put two separate methods in WebUtils for those two cases.

Dave

On Sun, Jul 15, 2012 at 06:48:15PM +0200, Andreas Raab wrote:

> Hi David -
>
> Are these patches needed only for IPv6 or also to support IPv4? If the
> latter, this would bother me greatly. I can understand why IPv6 may be
> implemented in incompatible ways but I can't see why there should be an
> incompatible interface to IPv4 addresses which have been used for eternity
> in Squeak. In particular given that (I believe) SocketAddress is a subclass
> of ByteArray it strikes me as not particularly hard to have Sockets support
> ByteArrays of length 4 as IPv4 socket addresses (and perhaps return
> instances of SocketAddress with length 4 or so). In any case, I'm not
> looking forward to seeing every last bit of networking code break - there
> has to be a better way...
>
> Cheers,
> ?? - Andreas
>
>
> >
> > -------- Original-Nachricht --------
> > Datum: Sun, 15 Jul 2012 11:46:54 -0400
> > Von: "David T. Lewis" <[hidden email]>
> > An: The general-purpose Squeak developers list
> > <[hidden email]>, [hidden email]
> > Betreff: Re: [squeak-dev] WebClient test fails in trunk
> >
> > On Wed, Jul 04, 2012 at 11:46:58PM +0200, Levente Uzonyi
> > wrote:
> > > On Tue, 3 Jul 2012, Frank Shearar wrote:
> > >
> > > >On 3 July 2012 17:42, Frank Shearar <[hidden email]> wrote:
> > > >>On 3 July 2012 17:38, Levente Uzonyi <[hidden email]> wrote:
> > > >>>On Tue, 3 Jul 2012, Frank Shearar wrote:
> > > >>>
> > > >>>>Hi,
> > > >>>>
> > > >>>>I took WebClient for a spin in a trunk image, and found a failing
> > > >>>>test: WebClientServerTest >> #testServerDestroy throws a
> > > >>>>"ConnectionClosed: Connection closed while waiting for data". That
> > > >>>>sounds familiar - I think this was introduced in a recent Network
> > > >>>>change.
> > > >>>>
> > > >>>>Still, it means WebClient needs fixing. I'll try fix it on my
> > evening
> > > >>>>commute, but if anyone feels like racing me, I'll be happy!
> > > >>>
> > > >>>
> > > >>>Interesting, I had no issue with it on windows, except for some
> > extension
> > > >>>methods trying to use the CrLf class variable in HTTPSocket (or some
> > > >>>other
> > > >>>class), but that variable doesn't exist.
> > > >>
> > > >>Which reminds me of an important fact I should mention: I'm testing
> > > >>this on an Ubuntu Lynx box.
> > >
> > > The VM also matters in this case. I tried it with an older CogVM which
> > > doesn't have the new socket primitives.
> >
> > I wanted to check that the recent network updates were not causing
> > problems for WebClient. I found that some updates to WebClient are
> > needed to account for the fact that network addresses are now being
> > represented as SocketAddress rather than ByteArray when running on
> > a VM that supports the new network primitives.
> >
> > Attached is a change set that addresses all the issues that I could
> > find. I'm assuming that the testServerDestroy issue discussed above
> > has already been addressed, as I had no failures for that test.
> >
> > From the preamble:
> >
> > Change Set: WebClient-networkPatches-dtl
> > Date: 15 July 2012
> > Author: David T Lewis
> >
> > These patches update WebClient to work with new network code in Squeak
> > trunk (all tests green).
> >
> > Explanation: When using the new network code along with a VM that
> > supports the
> > IPV6 primitives, socket addresses are instances of SocketAddress rather
> > than
> > ByteArray. Conversion of a socket address to a string must be
> > implemented
> > differently for SocketAddress.
> >
> > Also use String crlf instead of undeclared CrLf in HTTPSocket
> > class>>httpPostMultipart:args:accept:request:
> >
> > Note: Squeak automatically uses SocketAddress if the VM supports it at
> > image
> > start time. To force use of old style ByteArray addresses, evaluate
> > NetNameResolver useOldNetwork: true.
> >
> > Follow up: The WebClient methods in HTTPSocket are marked as overrides
> > (category
> > *WebClient-HTTP-override) but are not overrides with respect to Squeak
> > trunk.
> > The override category prevents these methods from appearing in a change
> > set
> > fileout. Should these methods just be adopted in trunk?
> >
> > Dave
> >
> >
>


Reply | Threaded
Open this post in threaded view
|

Re: WebClient test fails in trunk

David T. Lewis
On Mon, Jul 16, 2012 at 04:03:44PM +0200, Andreas Raab wrote:
> Hi David -
>
> The old IPv6 code. Sigh. Unfortunately, this code was written with the idea
> of being a complete rewrite and not an add-on to the existing library but
> it's now used as an add-on and this causes all sorts of problems, including
> major compatibility issues.

For sure it is an add-on, but backward compatibility is being preserved and
I suspect that you are overestimating the potential for problems.

Is anyone else having problems with the recent network updates in trunk?
I have not heard of any issues, but I'm not sure how much real use this
has gotten.

>
> I really do think that code needs serious overhaul. It basically needs to
> be rewritten such that:
> a) The external interface remains simply a byte array (be that an IPv4
> style 4-element byte array or some sort of SocketAddress which really *is*
> a socket address and not some plugin-internal structure combining address,
> port, family, and the phase of the moon)

I disagree. There is a wide range of possible address families and protocol
families, including X.25, Bluetooth, AF_LOCAL, DECnet, and others. Using
a raw byte array to directly represent an address does not make sense,
except for the limited (and limiting) case of a system that supports IPv4
and nothing else.

Furthermore, we can look to existing practice in Squeak, where the handles
for files, sockets, and surfaces are all represented as (theoretically)
opaque handles for which the relevant data structures are maintained by
the VM in the respective plugins. Given that a network address might be
one of many flavors of address, it makes perfect sense to use a similar
strategy for SocketAddress, in which the byte array value is intended to be
treated as an opaque handle, and the plugin is responsible for maintaining
the data associated with whatever than handle represents.

> b) SocketAddress[Info] is used privately within the socket and never
> exposed to a client unless that client specifically asks for it (but
> definitely not via stuff like Socket>>[local|remote]Address)

I'm not sure if I understand. The socket address info is exposed only in
the sense that the printString displays human readable descriptions. I do
not think this is fundamentally any different than displaying the four bytes
of a ByteArray separated by dots for an IPv4 address. But I think I must be
missing your point here.

> c) NetNameResolver needs to return IPv4 addresses by default and only
> answer IPv6 if the client specifically requests it.

I agree.

On my system, that is exactly what it does, at least with respect to as
answering IPv4 addresses by default:

  NetNameResolver addressForName: 'squeak.org' ==> 85.10.195.197(box2.squeakfoundation.org),0(0)
  NetNameResolver addressForName: 'localhost' ==> 127.0.0.1(localhost),0(0)

I did not try this on Windows or OS X. Is there a case in which you get an
IPv6 address when IPv4 was expected?

>
> Without this you're basically playing whack-a-mole trying to catch bugs
> that pop up randomly. A straightforward user like WebClient should be able
> to use Sockets without any change, defaulting to IPv4, and only when it
> specifically requests IPv6 support should it be exposed to IPv6. Otherwise
> this will be a complete nightmare.

The only real issue I saw in WebClient was conversion of socket addresses
(either ByteArray or SocketAddress) to host address strings.

Dave

>
> Cheers,
> ?? - Andreas
>
>
> >            
> > -------- Original-Nachricht --------
> > Datum: Sun, 15 Jul 2012 14:53:59 -0400
> > Von: "David T. Lewis" <[hidden email]>
> > An: Andreas Raab <[hidden email]>
> > CC: [hidden email]
> > Betreff: Re: [squeak-dev] WebClient test fails in trunk
> >
> >             Hi Andreas,
> >
> > The changes should hopefully work with any image regardless of whether
> > the new socket code is present, and regardless of what flavor of VM and
> > SocketPlugin are running.
> >
> > There should be no compatibity issues either for IPv4 or IPv6. The only
> > problem was that if the four-byte socket address is being represented by
> > a SocketAddress (subclass of ByteArray) rather than a ByteArray, then
> > converting it to a host name string with this does not work:
> >
> >   String streamContents:[:s| anAddress
> >     do:[:b| s print: b] separatedBy:[s nextPut: $.]]
> >
> > Aside from that, no real issues as far as I know (other than the trivial
> > undeclared CrLf issue).
> >
> > A SocketAddress is a kind of ByteArray, but it represents an opaque
> > reference to a data structure managed in the plugin. That data structure
> > does include the four-byte network address (for IPv4) but this should
> > not be directly referenced from the image.
> >
> > For example, on my PC with a 64 bit VM, the local address 127.0.0.2
> > is represented by a SocketAddress that, if printed out byte-by-byte,
> > would look like this:
> >
> >  '117.49.82.81.16.0.0.0.2.0.0.0.127.0.0.2.0.0.0.0.0.0.0.0'
> >
> > This is an 8 byte pointer, followed by the 32 bit network address,
> > followed
> > by some other stuff I don't remember. But the printString for this object
> > provides a more useful representation:
> >
> >  '127.0.0.2(linux-jh8m),0(0)'
> >
> > Thus for a network address represented in the traditional way, printing
> > it out as bytes separated by $. does the right thing, and for the same
> > address represented by a SocketAddress, the first portion of the
> > printString
> > up to $( is suitable. I did not check to see if this works with an IPv6
> > address, but it's good enough to cover the cases in the WebClient tests.
> >
> > I am guessing that for logging purposes, you would want to use the
> > complete
> > SocketAddress>>printString '127.0.0.2(linux-jh8m),0(0)' while for
> > converting
> > a socket address to a host string you would want only the address portion
> > '127.0.0.2', so I put two separate methods in WebUtils for those two
> > cases.
> >
> > Dave
> >
> > On Sun, Jul 15, 2012 at 06:48:15PM +0200, Andreas Raab wrote:
> > > Hi David -
> > >  
> > >  Are these patches needed only for IPv6 or also to support IPv4? If the
> >  
> > > latter, this would bother me greatly. I can understand why IPv6 may be  
> > > implemented in incompatible ways but I can't see why there should be an
> >  
> > > incompatible interface to IPv4 addresses which have been used for  
> > eternity
> > > in Squeak. In particular given that (I believe) SocketAddress is a
> > subclass
> > > of ByteArray it strikes me as not particularly hard to have Sockets
> > support
> > > ByteArrays of length 4 as IPv4 socket addresses (and perhaps return
> > > instances of SocketAddress with length 4 or so). In any case, I'm not
> > > looking forward to seeing every last bit of networking code break -
> > there
> > > has to be a better way...
> > >
> > >  Cheers,
> > >  ?? - Andreas
> > >
> > >  
> > > >            
> > > > -------- Original-Nachricht --------
> > > > Datum: Sun, 15 Jul 2012 11:46:54 -0400
> > > > Von: "David T. Lewis" <[hidden email]>
> > > > An: The general-purpose Squeak developers list
> > > > <[hidden email]>, [hidden email]
> > > > Betreff: Re: [squeak-dev] WebClient test fails in trunk
> > > >
> > > >             On Wed, Jul 04, 2012 at 11:46:58PM +0200, Levente Uzonyi
> > > > wrote:
> > > > > On Tue, 3 Jul 2012, Frank Shearar wrote:
> > > > >
> > > > > >On 3 July 2012 17:42, Frank Shearar <[hidden email]>
> > wrote:
> > > > > >>On 3 July 2012 17:38, Levente Uzonyi <[hidden email]> wrote:
> > > > > >>>On Tue, 3 Jul 2012, Frank Shearar wrote:
> > > > > >>>
> > > > > >>>>Hi,
> > > > > >>>>
> > > > > >>>>I took WebClient for a spin in a trunk image, and found a
> > failing
> > > > > >>>>test: WebClientServerTest >> #testServerDestroy throws a
> > > > > >>>>"ConnectionClosed: Connection closed while waiting for data".
> > That
> > > > > >>>>sounds familiar - I think this was introduced in a recent
> > Network
> > > > > >>>>change.
> > > > > >>>>
> > > > > >>>>Still, it means WebClient needs fixing. I'll try fix it on my
> > > > evening
> > > > > >>>>commute, but if anyone feels like racing me, I'll be happy!
> > > > > >>>
> > > > > >>>
> > > > > >>>Interesting, I had no issue with it on windows, except for some
> > > > extension
> > > > > >>>methods trying to use the CrLf class variable in HTTPSocket (or
> > some
> > > > > >>>other
> > > > > >>>class), but that variable doesn't exist.
> > > > > >>
> > > > > >>Which reminds me of an important fact I should mention: I'm
> > testing
> > > > > >>this on an Ubuntu Lynx box.
> > > > >
> > > > > The VM also matters in this case. I tried it with an older CogVM
> > which
> > > > > doesn't have the new socket primitives.
> > > >
> > > > I wanted to check that the recent network updates were not causing
> > > > problems for WebClient. I found that some updates to WebClient are
> > > > needed to account for the fact that network addresses are now being
> > > > represented as SocketAddress rather than ByteArray when running on
> > > > a VM that supports the new network primitives.
> > > >
> > > > Attached is a change set that addresses all the issues that I could
> > > > find. I'm assuming that the testServerDestroy issue discussed above
> > > > has already been addressed, as I had no failures for that test.
> > > >
> > > > From the preamble:
> > > >
> > > >   Change Set: WebClient-networkPatches-dtl
> > > >   Date: 15 July 2012
> > > >   Author: David T Lewis
> > > >  
> > > >   These patches update WebClient to work with new network code in
> > Squeak
> > > >   trunk (all tests green).
> > > >  
> > > >   Explanation: When using the new network code along with a VM that
> > > > supports the
> > > >   IPV6 primitives, socket addresses are instances of SocketAddress
> > rather
> > > > than
> > > >   ByteArray. Conversion of a socket address to a string must be
> > > > implemented
> > > >   differently for SocketAddress.
> > > >  
> > > >   Also use String crlf instead of undeclared CrLf in HTTPSocket
> > > > class>>httpPostMultipart:args:accept:request:
> > > >  
> > > >   Note: Squeak automatically uses SocketAddress if the VM supports it
> > at
> > > > image
> > > >   start time. To force use of old style ByteArray addresses, evaluate
> > > >   NetNameResolver useOldNetwork: true.
> > > >  
> > > >   Follow up: The WebClient methods in HTTPSocket are marked as
> > overrides
> > > > (category
> > > >   *WebClient-HTTP-override) but are not overrides with respect to
> > Squeak
> > > > trunk.
> > > >   The override category prevents these methods from appearing in a
> > change
> > > > set
> > > >   fileout. Should these methods just be adopted in trunk?
> > > >
> > > > Dave
> > > >
> > > >
> > >        
> >
>        

Reply | Threaded
Open this post in threaded view
|

Re: WebClient test fails in trunk

David T. Lewis
In reply to this post by Andreas.Raab
On Mon, Jul 16, 2012 at 04:03:44PM +0200, Andreas Raab wrote:

> Hi David -
>
> The old IPv6 code. Sigh. Unfortunately, this code was written with the idea
> of being a complete rewrite and not an add-on to the existing library but
> it's now used as an add-on and this causes all sorts of problems, including
> major compatibility issues.
>
> I really do think that code needs serious overhaul. It basically needs to
> be rewritten such that:
> a) The external interface remains simply a byte array (be that an IPv4
> style 4-element byte array or some sort of SocketAddress which really *is*
> a socket address and not some plugin-internal structure combining address,
> port, family, and the phase of the moon)
> b) SocketAddress[Info] is used privately within the socket and never
> exposed to a client unless that client specifically asks for it (but
> definitely not via stuff like Socket>>[local|remote]Address)
> c) NetNameResolver needs to return IPv4 addresses by default and only
> answer IPv6 if the client specifically requests it.
>
> Without this you're basically playing whack-a-mole trying to catch bugs
> that pop up randomly. A straightforward user like WebClient should be able
> to use Sockets without any change, defaulting to IPv4, and only when it
> specifically requests IPv6 support should it be exposed to IPv6. Otherwise
> this will be a complete nightmare.
>
Hi Andreas,

I had overlooked the #stringFromAddress: method in NetNameResolver, which
does the job of converting addresses to strings. This needed an update to
accomodate SocketAddress, which is fixed in trunk now. With that in place,
WebClient can use #stringFromAddress and the patches are trivial. Updated
changes attached.

Dave
 



WebClient-networkPatches-dtl.2.cs.gz (2K) Download Attachment