Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Review low level socket.send() #7

Open
miccoli opened this issue Jan 21, 2016 · 6 comments
Open

Review low level socket.send() #7

miccoli opened this issue Jan 21, 2016 · 6 comments

Comments

@miccoli
Copy link
Owner

miccoli commented Jan 21, 2016

The code for sending messages in protocol._OwnetConnection._send_msg is at present unsatisfactory.

After

sent = self.socket.send(header + payload)

a ShortWrite exception is raised if sent < len(header + payload) but it is not clear to me whether this socket.send should instead be retried in a while sent < len(header + payload): loop. If a retry loop is implemented, which are the conditions to break out of the loop to avoid deadlock?

A minor glitch to correct: the concat string header + payloadis constructed two times, one for sending, and one for measuring its length. Should optimise here.

@agentmnm
Copy link
Contributor

when does the exception occur? is it reproduce-able? how can one provoke the exception?

so far, pyownet is very well behaved in the environment I am using it.

@miccoli
Copy link
Owner Author

miccoli commented Jan 24, 2016

I've never seen a ShortWrite exception in production. However, reviewing the sockets code, I noticed that there is the theoretical possibility of aborting a _OwnetConnection._send_msg operation prematurely.

The fact is that socket sends are buffered at the OS level, so for the typical size of pyownet messages the socket.send returns immediately. The python library documentation states that

Applications are responsible for checking that all data has been sent; if only some of the data was transmitted, the application needs to attempt delivery of the remaining data.

The present code in _OwnetConnection._send_msg() does not retries to send after a failure, but aborts with a ShortWrite. I opened this issue as a reminder to myself to further investigate this point.

@miccoli miccoli changed the title Review socket errors on low level socket send Review low level socket.send() Jan 26, 2016
@miccoli miccoli added this to the non blocking sockets milestone Jan 26, 2016
@miccoli
Copy link
Owner Author

miccoli commented Jan 26, 2016

A possbile solution could be to abort a socket.send if the protocol-level time-out has expired.

@agentmnm
Copy link
Contributor

ok, I see.

If you don't mind, I would have a go at the while loop with time-out in socket-send...

So how would owserver deal with a an aborted socket.send? Does it itself have a time-out?

@miccoli
Copy link
Owner Author

miccoli commented Jan 28, 2016

Before looking for a solution I would like to have some evidence that we have a problem here. I've done some testing and socket.send blocks or sends less than expected bytes only under extreme conditions of network congestion, which should not be possible with owserver messages. Presently the danger is only to have am extremely rare ShortWrite that could be avoided by retrying the socket.send. (For socket.send the socket.settimeout(_SCK_TIMEOUT) applies, so I do not see the danger of a deadlock.) The present issue is just a reminder in case problems surface.

This issue is also a hint for a complete new implementation of _OwnetConnection in such a way that timeouts are possible not only at owserver pings but with a finer granularity. But before changing again the code I would like to see what happens with the present implementation.

@agentmnm
Copy link
Contributor

Ok, fair enough!
I would be game for more testing here too, once the heating period is over in end of march or april.
Then, I also want to try out Johan Ströms libftdi implementation for the LinkUSB.
I let You know, if I experience a ShortWrite or Timeout Exception in the meantime...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants