Wait for the full PDU before parsing#82
Conversation
code-of-kpp
left a comment
There was a problem hiding this comment.
Unfortunately some more changes required. We support non-blocking mode by calling this function just once when there is something to read from the socket.
So logic should be modified a bit to avoid while len(loop)` this can be done later in separate PR after this one got merged though.
|
Should we merge even though? |
|
Hmmm I'm not sure I get it (genuinely). If I understand correctly, the Then, when we get the length of the next pdu, we need all of it to keep working anyway, so we might just wait for it, no ? I'm not sure what you would like to do instead ? Maybe a failsafe to make sure we don't get stuck in that loop forever would be a good addition though (it would mean that the connection is not broken but we stop receiving anything at all). |
|
Yes, I also vote for merging that. Doesn't look like it breaks any interface. Actually, this is documented that recv may return less bytes than requested and the PR follows the advice from the docs |
|
@podshumok I'm merging that. If you still have objections, please raise a concern and I'll will revert or fix that |
socket.recv(4) might only get 1 or 2 or 3 bytes occasionally - in which case the library currently raises a "Broken PDU" error - so we might want to wait for the full 4 bytes there as well. See: https://stackoverflow.com/questions/59017085/socket-recv-receives-less-bytes-than-expected |
|
@stefanruijsenaars Yeah, actually, you're right. We need to take that into account. I wonder how it works now… 🤔 |
After playing and testing the lib, I noticed that under "stress" - sending around 10 SMS/s and receiving the DLRs on the same connection - I encountered failing PDUs out of nowhere.
I pinned the issue down to the fact that the current implementation expects all the data from a pdu to be in the buffer and sometimes, it isn't. WHen that's the case, it will retrieve less data than the expected length and it then messes all following PDUs (if it doesn't raise an exception before).
I first tried using the MSG_WAITALL flag but this didn't seem to work (surely something platform dependant) so I made that little loop which fixes everything on my side.
Let me know if I missed something, or if you need more information