added a break condition to _ctrl_read_forever

[evalott100]

Breaks out after a constant number of empty bytestrings have been received in a row.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.07%. Comparing base (8d95812) to head (e19d68c).
Report is 1 commits behind head on master.

❗ Current head e19d68c differs from pull request most recent head bd25115. Consider uploading reports for the commit bd25115 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #88      +/-   ##
==========================================
+ Coverage   96.90%   97.07%   +0.17%     
==========================================
  Files          12       12              
  Lines        1194     1196       +2     
==========================================
+ Hits         1157     1161       +4     
+ Misses         37       35       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[evalott100]

Tested against the real panda and PandABlocks/PandABlocks-ioc#108

[AlexanderWells-diamond]

Should have a unit test added?

[evalott100]

Should have a unit test added?

Agreed, that's it added

[AlexanderWells-diamond]

I am missing the context of these empty packets. What situation(s) cause them to be sent, and what should the behaviour in the rest of this system be when one is received? What about multiple (as this code seems to look for)?

A second question is what should the restart/retry behaviour be when the connection is terminated; when the ConnectionError exception is raised, that'll propogate out and presumably terminate all communication. Is the user expected to manually restart the connection at that point?

Also it may be worth writing a test that sends _EMPTY_PACKET_RETRIES - 1 packets, then a valid packet, then a few more empty packets to confirm that the counter resets correctly.

[coretl]

Context:

• When the PandA disconnects then recv will return an empty string
• The current code will then sit and spin eating all the CPU
• We want to break out of the loop if that happens, and in some way mark the client as "disconnected" so its obvious

@evalott100 I have never seen the connection come back when it gets in that state, did you add the retry code because of something you saw, or just in case?

[evalott100]

@evalott100 I have never seen the connection come back when it gets in that state, did you add the retry code because of something you saw, or just in case?

Nothing I noticed, it's just in case. Happy to take it out if you think it's unneccesary?

[AlexanderWells-diamond]

• We want to break out of the loop if that happens, and in some way mark the client as "disconnected" so its obvious

PythonSoftIOC #157 is currently under review that should allow this behaviour.

• The current code will then sit and spin eating all the CPU

The more I read around this the more I am believing that, as soon as we see an empty byte response, the connection is gone and is not coming back. I'm more used to using sockets directly, rather than asyncio's wrappers around them, but for a socket if you receive a "0" from a recv() call then the other end is disconnected. I can't see exactly what the asyncio StreamReader code is doing, but it seems to be interpreting "no data" as b'', which agrees with the documentation. So I believe that if we see a b'' response, we should just terminate immediately.

[coretl]

@evalott100 I have never seen the connection come back when it gets in that state, did you add the retry code because of something you saw, or just in case?

Nothing I noticed, it's just in case. Happy to take it out if you think it's unneccesary?

Yes please

[AlexanderWells-diamond]

Change looks good. How visible is the exception message when the disconnect occurs i.e. where does it get logged to?

[evalott100]

Change looks good. How visible is the exception message when the disconnect occurs i.e. where does it get logged to?

I'll add another loggging.error