BetfairStream doesn't set _running to False after a socket exception in _read_loop about betfair HOT 7 CLOSED

Ah nice find, you can see that under the try except there is some code which attempts to shutdown the socket. Regarding the pull it might be cleaner if it called self.stop() unless you disagree?

from betfair.

I clearly wasn't thinking when I added that finally block - the loop is only ever going to get executed once with that in there.

I agree about calling the stop method instead.

I've changed the raises to just logging.error(...) calls and then added self.stop().

I changed the test_read_loop_error test to call _read_loop() and then assert not self.betfair_stream._running - not sure if this is the best way to test it?

If the raise is removed and changed to a logging.error call, it means that exceptions.SocketError is actually no longer used anywhere in the package; do you want to get rid of it?

When this PR gets merged, I'll update the logging PR to switch the naked logging.error call to obtain a module logger and then use that.

from betfair.

I need an error to be raised if there is a timeout or another error or my framework will not know that there is an issue and the stream needs to be restarted / reconnect. I agree with the addition of self.stop() but I think an error has to be raised.

from betfair.

I think I misunderstood your reason for calling stop() - I thought it was so that it would break out of the loop and call the socket cleanup code.

Clearly if the exception is still raised, it will never reach that cleanup code even after calling stop() and having _running set to False.

However, just calling stop() before raise will set _running to False which will allow me to check the aliveness of the thread, and still throw the exception.

Shall I just create a new branch / PR and put that in there since there's a load of rubbish which isn't applicable now in this one?

from betfair.

Hmm you are right raising the error means the socket will never be cleaned unless the entire while loop is placed in a try/except and a finally with the stop + cleanup code but that doesn't seem very pythonic.

I think the logical thing to do is to move the clean up code into stop(), because the socket gets recreated during the start() process anyway.

from betfair.

Agreed, I think the socket cleanup code should go into stop().

from betfair.

I've redone the PR in #52

from betfair.