Implement Outstanding Operations management#99
Conversation
|
That's the wrong implementation... Will fix it later, I gotta go. |
|
It can only work in a threaded environment (threaded listen/poll). I'm sure it isn't the best way to implement this, by far, but it gives a good idea of what needs to be done I think. I'd like to ear your ideas on this @eigenein when you have time. |
|
Hi @Lynesth, sorry for the slow reaction, had tough time recently, just wanna keep you updated. I'll take a look within a week, most likely this Friday |
| consts.DESCRIPTIONS[consts.SMPP_ESME_RINVBNDSTS], | ||
| )) | ||
|
|
||
| if self.max_outstanding_operations and p.command[-5:] != '_resp': |
There was a problem hiding this comment.
It's better to use pdu.is_request() and pdu.is_response() to tell if it's a response
There was a problem hiding this comment.
Didn't notice those functions it'll be better indeed.
| raise exceptions.ConnectionError() | ||
| sent += sent_last | ||
|
|
||
| if p.command[-5:] == '_resp': |
There was a problem hiding this comment.
Same here for is_response() test
| else: | ||
| self.outstanding_operations += 1 |
There was a problem hiding this comment.
Mmm, I don't really get this part. Why reading a PDU really increases the number?
There was a problem hiding this comment.
In this case we are reading a PDU which isn't a response so it's the SMSC asking for something, so it is an outstanding operation until we actually answer it.
| self.port = int(port) | ||
| self._socket = socket.socket(socket.AF_INET, socket.SOCK_STREAM) | ||
| self.timeout = timeout | ||
| self.outstanding_operations = 0 |
There was a problem hiding this comment.
I'm a bit afraid of the counter getting out of sync for requests/responses. Would it be smarter to keep a set of PDU sequence IDs? So, when you got a request PDU then you add its sequence ID to the set. For a response PDU you remove its sequence ID from the set. And to check you test length of the set. Then we're more safe in terms of lost packets and/or retries
By the way, I guess that unbind and disconnect handlers should reset the current outstanding operations counter/set. Otherwise, we may never get response for some unconfirmed PDUs
There was a problem hiding this comment.
I think using a set is a good idea yes.
Regarding unbind/disconnect, it shouldn't reset it. Example:
I connect, send 10 submit_sm and then disconnect. When I reconnect 5 minutes later, the SMSC will actually send me 10 submit_sm_resp that were outstanding, using the same sequence numbers that were used to send the submit_sm.
We don't reset the sequence number on unbind/disconnect by default either, do we ?
Nevertheless it wouldn't be failproof in case the script gets killed or something so...
|
|
||
| if self.max_outstanding_operations and p.command[-5:] != '_resp': | ||
| while self.outstanding_operations >= self.max_outstanding_operations: | ||
| sleep(.1) |
There was a problem hiding this comment.
I'd probably use threading.Event.wait() to block here. Then response handling code could flag the event to unblock the loop here. This way I wouldn't need the sleep
There was a problem hiding this comment.
Never used events before, will take a look a that, thank you.
|
@eigenein Thank you for your comments, I will answer to each individually. |
| except KeyError: | ||
| self.logger.warning('Failed to unregistered outstanding operation {}'.format(operation)) | ||
| else: | ||
| operation = '{}{}'.format('C' if sending else 'S', pdu.sequence) |
There was a problem hiding this comment.
Duplicated line which is used in both branches. Could you move it right above the if?
| """Send PDU to the SMSC""" | ||
|
|
||
| if self.state not in consts.COMMAND_STATES[p.command]: | ||
| if self.state not in consts.COMMAND_STATES[pdu.command]: |
There was a problem hiding this comment.
Thanks for renaming that :)
According to the SMPP specification:
This is how I implemented it for my needs. It seems to work fine on my end (~1500 SMS / day on average).
Let me know @eigenein @podshumok what you think about it.