[Edonkey-java-devel] RE: TODO: gui-core connection packet handling

[Robin <ke...@if...>]

> > CLC: already known command line client or Core
> > EJC: Edonkey Java Controler
> > EGC: Edonkey GTK Controler
> > GUI: generic Java or GTK (or PHP) Controler
>
> ok, I'm not used to those, but I'll try to adhere
> to them as much as possible
> ;-)

Thanks, you don't have to if you don't want, if you are used
to others shortcuts let me know we can use them. But using
such abbreviations is always a little annoying in the
beginning, but always useful after.

> > It seems that you are right, the communication
> and packet
> > handling is somehow...bad.
> > It's as you describe:
>
> hmm, I think that should be stuff that should be
> on the list, if you ask me.
> As you replied privately though, I'll reply
> privately as well. You can then
> decide if you want to forward it or not (in whole
> or only parts) ;-)

Don't pay attention, it was a mistake, i wanted to write to
the list.

> I had a quick look at CoreController.java in
> jc/src/com/mm/net/, and found
> basically only the following issue:
>
> * how does the EJC behave if the packet size
> doesn't match the size read?
>
>    (a) if read > packet size: not sure what
> happens. This case should not
>         happen though.
>
>    (b) if packet size > read: line 745 should
> show a message
>         "size is not match with actual read"
> (sic). The problem that I see is
>         that the EJC might not read in those
> 'superfluous' bytes, nor will it
>         skip those bytes. But maybe I'm just wrong :-)
>         A simple 3 lines here could resolve quite
> a few problems (see below).

I have to go deeper in the code of the MediaInputStream to
see how it works, I'll answer more precisely after.

> > [MetaInputStream stuff:]
> > As you can see, the message is parsed in several times:
> > First the Header, than the size (in 4
> consecutive read), and
> > then the message id, and after, the contents in several
> > times.
> > It seems clear that your advice and method is
> better, but I
> > don't know Java enough (I'll look at this in
> detail later)
> > to know if a single read on the socket would deliver a
> > packet in "one shot" (I'm more experienced with
> C), I should
> > investigate this.
> > But whatever the method is, reading the header, than the
> > size, than the payload should work in both case
> (reading a
> > block, or reading byte after byte). I suspect a
> bug either
> > in the CLC, either in the GUI (I suspect the GUI).
>
> Maybe we should leave that stuff alone then for
> now, and rather fix the stuff
> as it is, that might be easier?

Yes, packets in error are:
Got MessageID: 199 but size is 123
Got MessageID: 262 but size is 305527903
Got MessageID: 188 but size is 107
Got MessageID: 188 but size is 119

If you have the exact format of this packets, it will save
me the time to look at the EGC code.

> > I'd prefer to just correct a bug, or adjust a
> function that
> > read a certain type of message than to rewrite the whole
> > packet reading code (this could come in a second time).
>
> yes, agreed. Just ignore my first mail to the
> list then :-)

No problem, incoming packet handling code will be rewritten
after.

> > PS: look at what I find after activating the trace code:
> > Size is not match with actual read: MessageID:
> 199 size: 123
> > actual read: 108
> > ?(-76)
> > Retrieving
> >
> http://sda.edonkey2000.com/ver/ver.php?ui=2&ver=1.
> 2.0&core=1
> > 0481
> > (0)(0)(0)(28)(4)(3)(0)
> > the parenthesis correspond to this code:
> > MessageUtils.trace(new String("" + (char) header + "(" +
> > header + ")"));
> >
> > We found a bug, I'll try to find it with more
> precision, and
> > i'll let you know
>
> aaah. The newer cores send an options message as
> one of the first messages
> when the GUI connects. So if the GUI fails here,
> everything else will be
> unlikely to work, as the EJG apparently does not
> re-sync the input stream to
> the next packet header.
>
> Try inserting this code in CoreController.java
> from line 735 and see if that
> makes it work:
>
>             if (!processed) {
>                 byte[] result = new byte[size];
>                 int read = 0;
>                 while (read < size)
>                     read += in.read(result, read,
> size - read);
>                 MessageUtils.trace(new
> String(result) + "\n");
>             }
>             else {
>                 int end = in.getCount();
> +              byte[] resttoignore = new byte[end-read+1];
> +              while (read < size)
> +                   read += in.read(resttoignore,
> read, size - read);
>                 if (end - start != size) {
>                     MessageUtils.trace("Size is
> not match with actual read:
> MessageID: " + (256 + msgID) + " size: " + size +
> " actual read: " + (end -
> start) + "\n");
>                 }
>             }
>             in.setCount(0);
>
> or similar (I've never coded java, so this might
> be plain wrong). What it's
> supposed to do is just read in the extra X bytes
> of the packet into a buffer
> and ignore them.

The code was ok (there is just the declaration of int read
missing), and fooly copying it failed with "out of array"
exceptions.
If the data read is longer than expected, the superflous
bytes should be caught by the read thread loop:

if (header == Packet.HEADER_CHAR) {
	...
	dispatchMessage(_in.readByte(), size - 1, _in);
}
// If the first byte is not a packet HEADER (doh!)
else
{
	MessageUtils.trace(new String("" + (char) header + "(" +
header + ")"));
}

After dispatching a too long message, the else print the
superflous bytes and then, the next header should be read.
Once again I have to understand the reading code, it will be
clearer after.

> > > [snip]
> >
> > Huh, as I said, whatever it receives, if the
> CLC respects
> > his own protocol (HEADER-SIZE-MSGID-PAYLOAD),
> there should
> > be no "out of sync" situation. May I suspect
> the CLC not to
> > respect this simple protocol ?
> > the message: Size is not match with actual
> read: MessageID:
> > 199 size: 123 actual read: 108
> > seems to confirm the fact, isn't it ?
>
> No, the CLC respects this protocol. The CLC will
> always produce packets where
> a header that says 'size=X' is followed by X
> bytes of data. Always :-)
>
> In this case it's just that the options message
> (199) has been extended by me
> by a few bytes. So the core sends a header that
> says 'size=123', but the EJG
> only expects 108 bytes, because it doesn't know
> about my extensions yet. But
> instead of skipping the 15 bytes it doesn't know
> what to do with it just
> prints out a message and goes on parsing at those
> unexpected bytes, expecting
> a new packet header there. At least if I'm
> correct. And that would
> definitively be a bug in the Java client....

At first glance, i would say that it reads the 108 bytes,
and pass the 15 bytes remaining that are not a "header
byte". What can goes wrong is if it reads the header, the
108 bytes expected, and find a HEADER byte in the 15
remaining bytes. This could lead to read the 4 length byte
and skip important data...

I stop this mail, and go in the code.

I'll keep you informed.

Robin