Exception in the socket code

Hi Paolo,

from time to time I get the below exception. I fail to properly reproduce it
with a standalone test case though.

FileDescriptor)>>nextAvailable:into:startingAt:
...
        self isOpen ifFalse: [^available].

^^^ just checks if the fd is >= 0

        count := self
                    fileOp: 3
                    with: aCollection
                    with: position + available
                    with: (position + n - 1 min: aCollection size).

^^^ okay an error happened here that will not throw an exception and the
default fileOp:with:with:with will return nil. So maybe we should add an
ifFail: [0] to it?

        count := count + available.


Exception

Object: nil error: did not understand #+
MessageNotUnderstood(Exception)>>signal (ExcHandling.st:254)
UndefinedObject(Object)>>doesNotUnderstand: #+ (SysExcept.st:1407)
Sockets.TCPSocketImpl(FileDescriptor)>>nextAvailable:into:startingAt:
(FileDescr.st:802)
optimized [] in Sockets.StreamSocket>>newReadBuffer:
(Sockets.star#VFS.ZipFile/Sockets.st:1323)
Sockets.ReadBuffer>>atEnd (Sockets.star#VFS.ZipFile/Buffers.st:129)
Sockets.StreamSocket>>peek (Sockets.star#VFS.ZipFile/Sockets.st:1267)
Sockets.StreamSocket>>atEnd (Sockets.star#VFS.ZipFile/Sockets.st:1152)
Sockets.StreamSocket(Stream)>>nextLine (Stream.st:215)

_______________________________________________
help-smalltalk mailing list
[hidden email]
http://lists.gnu.org/mailman/listinfo/help-smalltalk

On 03/25/2011 01:00 PM, Holger Hans Peter Freyther wrote:
The correct #ifFail: would probably be to check errno.

We should also look for cases where raising an exception is appropriate
instead of calling the failure block.

Paolo

_______________________________________________
help-smalltalk mailing list
[hidden email]
http://lists.gnu.org/mailman/listinfo/help-smalltalk

On 03/25/2011 01:08 PM, Paolo Bonzini wrote:

> The correct #ifFail: would probably be to check errno.
>
> We should also look for cases where raising an exception is appropriate
> instead of calling the failure block.

When the fileOp primitive fails we still go through File>>#checkError to at
least check the errno. This means that in this case the errno was not set, I
agree that it would be nice to know how the read has failed but the problem is
hard to reproduce (and means dropping out of vpn).

regards
        holger

_______________________________________________
help-smalltalk mailing list
[hidden email]
http://lists.gnu.org/mailman/listinfo/help-smalltalk

On Fri, Mar 25, 2011 at 14:33, Holger Hans Peter Freyther
<[hidden email]> wrote:
Are you sure the problem isn't in the arguments to #nextAvailable:?

Paolo

_______________________________________________
help-smalltalk mailing list
[hidden email]
http://lists.gnu.org/mailman/listinfo/help-smalltalk

On 03/25/2011 02:38 PM, Paolo Bonzini wrote:

> Are you sure the problem isn't in the arguments to #nextAvailable:?

Yes, besides an off by one in the source line the below causes the exception:

        count := count + available.

Object: nil error: did not understand #+
MessageNotUnderstood(Exception)>>signal (ExcHandling.st:254)
UndefinedObject(Object)>>doesNotUnderstand: #+ (SysExcept.st:1407)

so I assume count is nil and this seems to be the case when the fileOp
primitive is failing.

_______________________________________________
help-smalltalk mailing list
[hidden email]
http://lists.gnu.org/mailman/listinfo/help-smalltalk

On 03/25/2011 02:43 PM, Holger Hans Peter Freyther wrote:
I mean, are you sure the primitive isn't failing because for example
aCollection is bogus?  I mean, this crashes the VM:

st> (FileDescriptor on: 0) nextAvailable: 1 into: 123 startingAt: 1

stdin:4: Aborted
(ip 80)FileDescriptor>>#nextAvailable:into:startingAt:
(ip 14)UndefinedObject>>#executeStatements
(ip 0)<bottom>
Aborted

:)

Paolo

_______________________________________________
help-smalltalk mailing list
[hidden email]
http://lists.gnu.org/mailman/listinfo/help-smalltalk

On 03/25/2011 02:59 PM, Paolo Bonzini wrote:

> I mean, are you sure the primitive isn't failing because for example
> aCollection is bogus?  I mean, this crashes the VM:

hmm... I assume it is valid as I go through the normal Socket>>#atEnd.



should the vm crash? patches welcome?


_______________________________________________
help-smalltalk mailing list
[hidden email]
http://lists.gnu.org/mailman/listinfo/help-smalltalk

On 03/25/2011 03:11 PM, Holger Hans Peter Freyther wrote:
> On 03/25/2011 02:59 PM, Paolo Bonzini wrote:
>
>> I mean, are you sure the primitive isn't failing because for example
>> aCollection is bogus?  I mean, this crashes the VM:
>
> hmm... I assume it is valid as I go through the normal Socket>>#atEnd.

I guess I didn't explain myself very well...

st> (FileDescriptor on: 0) nextAvailable: 1 into: nil startingAt: 1

Object: nil error: did not understand #+
MessageNotUnderstood(Exception)>>signal (ExcHandling.st:254)
UndefinedObject(Object)>>doesNotUnderstand: #+ (SysExcept.st:1407)
FileDescriptor>>nextAvailable:into:startingAt: (FileDescr.st:802)
UndefinedObject>>executeStatements (a String:1)
nil

Does this patch help, or otherwise change the failure?

diff --git a/packages/sockets/Buffers.st b/packages/sockets/Buffers.st
index 8eb4dcd..2084797 100644
--- a/packages/sockets/Buffers.st
+++ b/packages/sockets/Buffers.st
@@ -46,6 +46,12 @@ collection.'>
  ptr := 1
      ]

+    close [
+ <category: 'buffer handling'>
+        super close.
+        flushBlock := nil
+    ]
+
      flushBlock: block [
  "Set which block will be used to flush the buffer.
  The block will be evaluated with a collection and
@@ -119,6 +125,12 @@ evaluates an user defined block to try to get some
more data.'>
     yourself "Force a buffer load soon"
      ]

+    close [
+ <category: 'buffer handling'>
+        super close.
+        fillBlock := nil
+    ]
+
      atEnd [
  "Answer whether the data stream has ended."

diff --git a/packages/sockets/Sockets.st b/packages/sockets/Sockets.st
index ea11feb..c6358cd 100644
--- a/packages/sockets/Sockets.st
+++ b/packages/sockets/Sockets.st
@@ -1318,6 +1318,7 @@ This class adds a read buffer to the basic model
of AbstractSocket.'>
  <category: 'private - buffering'>
  ^(ReadBuffer on: (String new: size)) fillBlock:
  [:data :size || n |
+                data isNil ifTrue: [self halt].
  self implementation ensureReadable.
  n := self implementation isOpen
     ifTrue: [self implementation nextAvailable: size into: data
startingAt: 1]
@@ -1459,6 +1460,7 @@ This class adds read and write buffers to the
basic model of AbstractSocket.'>
  ^(WriteBuffer on: (String new: size)) flushBlock:
  [:data :size |
  | alive |
+                data isNil ifTrue: [self halt].
  self implementation ensureWriteable.
  alive := self implementation isOpen
     and: [(self implementation next: size putAll: data startingAt:
1) > -1].



I'll apply the Buffers.st part since it's good anyway.

> should the vm crash? patches welcome?

no, it shouldn't.  patch on the way.


_______________________________________________
help-smalltalk mailing list
[hidden email]
http://lists.gnu.org/mailman/listinfo/help-smalltalk

On 03/25/2011 03:23 PM, Paolo Bonzini wrote:
he normal Socket>>#atEnd.
>
> I guess I didn't explain myself very well...

sorry, more me being jet-lagged fro my last flight.

>
> st> (FileDescriptor on: 0) nextAvailable: 1 into: nil startingAt: 1
>
> Object: nil error: did not understand #+
> MessageNotUnderstood(Exception)>>signal (ExcHandling.st:254)
> UndefinedObject(Object)>>doesNotUnderstand: #+ (SysExcept.st:1407)
> FileDescriptor>>nextAvailable:into:startingAt: (FileDescr.st:802)
> UndefinedObject>>executeStatements (a String:1)
> nil

okay, looks very similar. I am not able to reproduce this right now. :)


>
> I'll apply the Buffers.st part since it's good anyway.

hmm. does this mean I can't read data out of a remotely closed socket? Or only
when I have called close myself. Sorry for the stupid question.


_______________________________________________
help-smalltalk mailing list
[hidden email]
http://lists.gnu.org/mailman/listinfo/help-smalltalk

On 03/25/2011 03:28 PM, Holger Hans Peter Freyther wrote:
>> >
>> >  I'll apply the Buffers.st part since it's good anyway.
> hmm. does this mean I can't read data out of a remotely closed socket? Or only
> when I have called close myself. Sorry for the stupid question.

No, only when you have called close yourself.  In fact, I'm not sure
what would call #close on the buffers; nothing, probably.  But the patch
would definitely tell us if that's happening.

Now that I think of it, while my patch to the buffer classes is good for
GST, you may want to replace the two implementations of #close with just
"self halt".

Paolo

_______________________________________________
help-smalltalk mailing list
[hidden email]
http://lists.gnu.org/mailman/listinfo/help-smalltalk

On 03/25/2011 03:33 PM, Paolo Bonzini wrote:
Okay,

I can reproduce the issue using tcpkill of dsniff on the connection. I will
debug tomorrow morning. While testing I think I stumbled across mem corruption
of DoIt's (Eval [] in a file) and doing CTRL+C (e.g. when waiting for a
socket...). It is causing a segfault when clearing the obstack. :)

_______________________________________________
help-smalltalk mailing list
[hidden email]
http://lists.gnu.org/mailman/listinfo/help-smalltalk

On 03/25/2011 08:29 PM, Holger Hans Peter Freyther wrote:
> Okay,
>
> I can reproduce the issue using tcpkill of dsniff on the connection. I will
> debug tomorrow morning. While testing I think I stumbled across mem corruption
> of DoIt's (Eval [] in a file) and doing CTRL+C (e.g. when waiting for a
> socket...). It is causing a segfault when clearing the obstack.:)

master or stable-3.2 or?...

Paolo

_______________________________________________
help-smalltalk mailing list
[hidden email]
http://lists.gnu.org/mailman/listinfo/help-smalltalk

On 03/25/2011 08:35 PM, Paolo Bonzini wrote:

> master or stable-3.2 or?...

stable-3.2 but I don't have a test case for this yet. The crash is looking
like this though:

#0  0x005c3424 in __kernel_vsyscall ()
#1  0x00a432f1 in raise (sig=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
#2  0x00a44d5e in abort () at abort.c:92
#3  0x001f4a95 in oldspace_sigsegv_handler (fault_address=0x5, serious=0) at
oop.c:938
#4  0x002486d4 in sigsegv_handler (sig=11, sc=...) at handler-unix.c:134
#5  <signal handler called>
#6  obstack_free (h=0xbfdf2be4, obj=0x0) at obstack.c:366
#7  0x001e3c88 in _gst_free_tree () at tree.c:498
#8  0x001df066 in parse_doit (p=0xbfdf3568, fail_at_eof=<value optimized out>)
at gst-parse.c:473
#9  0x001dfb94 in parse_chunks (p=0xbfdf3568) at gst-parse.c:364
#10 0x001e00f7 in _gst_parse_chunks () at gst-parse.c:341
#11 0x001e1bb4 in _gst_parse_stream (method=false) at lex.c:1186
#12 0x0020d372 in _gst_process_stdin (prompt=0x804954d "st> ") at input.c:818
#13 0x0804936d in main (argc=0, argv=0x0) at main.c:415



_______________________________________________
help-smalltalk mailing list
[hidden email]
http://lists.gnu.org/mailman/listinfo/help-smalltalk

On 03/25/2011 03:33 PM, Paolo Bonzini wrote:

>
> Now that I think of it, while my patch to the buffer classes is good for GST,
> you may want to replace the two implementations of #close with just "self halt".

turns out that sockets use a different set of prims to read/write and that the
primitive does not set the errno like the 'normal' one does. My test case
right now is to connect a remote IP... and after this patch I do get a
connection refused File error.

diff --git a/libgst/prims.def b/libgst/prims.def
index db3254d..efd83b2 100644
--- a/libgst/prims.def
+++ b/libgst/prims.def
@@ -5861,6 +5861,8 @@ primitive VMpr_FileDescriptor_socketOp [succeed,fail]
 #endif

  fail:
+  if (errno)
+    _gst_set_errno (errno);
   PRIM_FAILED;

  succeed:

_______________________________________________
help-smalltalk mailing list
[hidden email]
http://lists.gnu.org/mailman/listinfo/help-smalltalk

On 03/25/2011 09:54 PM, Holger Hans Peter Freyther wrote:
Right, because I thought I'd use SO_ERROR.  Good catch, thanks!

Paolo

_______________________________________________
help-smalltalk mailing list
[hidden email]
http://lists.gnu.org/mailman/listinfo/help-smalltalk

On 03/25/2011 09:56 PM, Paolo Bonzini wrote:

>
> Right, because I thought I'd use SO_ERROR.  Good catch, thanks!

thanks, there is still an issue with using tcpkill and getting:

recvfrom(4, 0xb35bf7bc, 1024, 0, 0, 0)  = -1 ECONNRESET (Connection reset by peer)


UndefinedObject(Object)>>doesNotUnderstand: #+ (SysExcept.st:1407)
Sockets.TCPSocketImpl(FileDescriptor)>>nextAvailable:into:startingAt:
(FileDescr.st:800)
optimized [] in Sockets.StreamSocket>>newReadBuffer:
(Sockets.star#VFS.ZipFile/Sockets.st:1323)
Sockets.ReadBuffer>>atEnd (Sockets.star#VFS.ZipFile/Buffers.st:129)
Sockets.StreamSocket>>peek (Sockets.star#VFS.ZipFile/Sockets.st:1267)
Sockets.StreamSocket>>atEnd (Sockets.star#VFS.ZipFile/Sockets.st:1152)
Sockets.StreamSocket(Stream)>>nextLine (Stream.st:215)

_______________________________________________
help-smalltalk mailing list
[hidden email]
http://lists.gnu.org/mailman/listinfo/help-smalltalk

On 03/25/2011 10:01 PM, Holger Hans Peter Freyther wrote:

> recvfrom(4, 0xb35bf7bc, 1024, 0, 0, 0)  = -1 ECONNRESET (Connection reset by peer)
>


okay, 945cca8737d716e2436aaa064c2c6aed02d34627 of 11th of August 2008
introduced the following to cint.c:get_errno():

if (old == ESHUTDOWN || old == ECONNRESET
    || old == ECONNABORTED || old == ENETRESET)
   return 0;


this explains why the connection refused is working but the connection reset
(due using tcpkill) will fail. The question is how to go forward? So somehow I
want to get a EndOfStream or a FileError.

_______________________________________________
help-smalltalk mailing list
[hidden email]
http://lists.gnu.org/mailman/listinfo/help-smalltalk

On 03/26/2011 11:09 AM, Holger Hans Peter Freyther wrote:
Yes, your original idea of returning 0 now is correct.  checkError will
return 0 for you:

diff --git a/kernel/FileDescr.st b/kernel/FileDescr.st
index f424cd2..d2291d1 100644
--- a/kernel/FileDescr.st
+++ b/kernel/FileDescr.st
@@ -797,7 +797,8 @@ do arbitrary processing on the files.'>
     fileOp: 3
     with: aCollection
     with: position + available
-    with: (position + n - 1 min: aCollection size).
+    with: (position + n - 1 min: aCollection size)
+                    ifFail: [self checkError].
  count := count + available.
  count = 0 ifTrue: [atEnd := true].
  ^count
@@ -817,7 +818,8 @@ do arbitrary processing on the files.'>
     fileOp: 2
     with: aCollection
     with: cur
-    with: last.
+    with: last
+                            ifFail: [self checkError].
  result = 0 ifTrue: [^cur - position].
  cur := cur + result].
  ^cur - position

(untested).

Paolo

_______________________________________________
help-smalltalk mailing list
[hidden email]
http://lists.gnu.org/mailman/listinfo/help-smalltalk

On 03/26/2011 11:11 AM, Paolo Bonzini wrote:

> Yes, your original idea of returning 0 now is correct.  checkError will return
> 0 for you:


What about the following?

> +                    ifFail: [self checkError].

                       ifFail: [SystemExceptions.EndOfStream signalOn: self]


E.g. if I use socket nextLine I will now get a '' as the result that I would
need to handle specially. I am talking to a server right now and I know that
it needs to send 'username: ' right now and my code is doing something like:

[
tmp := socket nextPart. "like nextLine but also checks for $:"
tmp = 'username' ifFalse: [^self error: '']
] on: SystemExceptions.EndOfStream do: [].


With the proposed change to add ifFail: [self checkError] I will now either
need to do:

[
] on: SystemExceptions.EndOfStream do: []
  on: Error do: [].

or

tmp := socket nextPart.
(tmp isEmpty and: [socket atEnd]) ifTrue: [graceful exit]

or move the graceful part into nextLine and nextPart, and would need to
differentiate if nextLine has actually read anything at all.


what do you think?

_______________________________________________
help-smalltalk mailing list
[hidden email]
http://lists.gnu.org/mailman/listinfo/help-smalltalk

On 03/26/2011 11:23 AM, Holger Hans Peter Freyther wrote:  >
> what do you think?

What about changing ^0 to "self pastEnd. ^0" in checkError?

Paolo

_______________________________________________
help-smalltalk mailing list
[hidden email]
http://lists.gnu.org/mailman/listinfo/help-smalltalk