From: Tim <t....@ze...> - 2003-05-17 14:22:17
|
On Friday 16 May 2003 20:51, Robin wrote: Hi, > I find a little time to work on the EJC, ran through the > startup code, up to the serious things: the CoreController > Class, the one that handle the communication between the GUI > and the CLC. > > It seems that you are right, the communication and packet > handling is somehow...bad. > It's as you describe: 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). > [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? > 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 :-) > 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. > > [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.... cheers -Tim |
From: Robin <ke...@if...> - 2003-05-17 14:39:08
|
> > 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 |
From: William P. R. I. <big...@co...> - 2003-05-18 02:47:20
|
> > [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 :->) If the packets that the core sends out to the GUI are always correct according to the 2 bytes for length, then there shouldn't ever be an out of sync problem if the EJC is properly coded. I just got back from Cornell, so I've got to finish unpacking and such, but since I don't start work for another week I'll futz around with the packet handling code next week. --William |
From: Robin <ke...@if...> - 2003-05-18 11:09:42
|
> > > [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 :->) > > If the packets that the core sends out to the GUI > are always correct > according to the 2 bytes for length, then there > shouldn't ever be an out > of sync problem if the EJC is properly coded. Yes there is, in fact, the problem is in the end of the dispatchMessage switch, the patch sent by Tim was good but not enough to solve the problems. At the end of the switch, if the packet is not "processed", it flush the packet, but if the packet is "processed", it displays a message "size mismatch", but doesn't flush, I continue working on it, keeping the first Tim Patch. > I just got back from Cornell, so I've got to > finish unpacking and such, > but since I don't start work for another week > I'll futz around with the > packet handling code next week. I hope it will be fixed at this time, but the more we'll be, the better it will be. See you, Robin |