Closed RealOrangeOne closed 6 years ago
Looks good to me. Are the tests failing because of this PR or are they currently broken?
Looks like it's because theyre broken? cc @Adimote
Although this PR's diff looks good to me, the tests are failing (and not on master), so I'd prefer we didn't merge this until this is fixed.
Having said that, I can't for the life of me figure out why they're failing. They seem to be fine in 80f190f (more or less, there are two failures out of sixteen), which is the first commit in this PR; however in the immediately following commit, e520f04, every single test produces an error.
After a bit of investigation, my best guess as to what is causing the error scenario is:
_connect
calls receive
receive
calls _recv_from_socket
using the _socket_with_single_retry
wrapper_recv_from_socket
times out_socket_with_single_retry
attempts to reconnect the socket and retry the operation_connect
socket.connect
thus fails with a "Resource temporarily unavailable" exceptionPerhaps this could be fixed by not allowing the receive
to retry when it is called by _connect
? receive
appears to already have a parameter should_retry
, so this should be an easy fix (if it is indeed the right way to approach this).
However I have no clue how (if?) this is caused by the changes in e520f04. After some trial-and-error I've found that applying this change on top of this PR doesn't result in the error scenario above being triggered, and all tests pass (though the linting then fails). This change constitutes going back to using a str
for socket_path
rather than a Path
. 100 brownie points will be awarded to whoever can explain why this patch works.
tl;dr: it seems wrapping socket_path
in a Path
somehow causes socket reads to time out and results in all tests failing
Rather than always re-defining the same serial logic in each subclass of
Board
, move it up intoBoard
.