Clarify documentation for teardown return from run_forever()

[fujiapple852]

The doc-comment in run_forever says:

teardown: bool
            False if caught KeyboardInterrupt, True if other exception was raised during a loop

This doesn't cover are the cases where run_forever returns None.

One case is on signal SIGTERM which causes keep_running to become False and therefore SSLDispatcher.read returns None. Pressing the stop button in PyCharm will trigger this.

I think the intention here is to allow the caller to differentiate between a graceful and non-graceful exit. Given that it may be sensible to have run_forever always return either False or True and have both SIGINT (i.e. KeyboardInterrupt exception) and other non-exceptional status (i.e. SIGTERM) return False.

[engn33r]

If I am understanding correctly, there is an edge case that is triggered when pressing the stop button in an IDE. I haven't used PyCharm lately so this could be an error that I never encountered. Could you try drafting a PR so that "False" is returned instead of "None" in this edge case? It should help me understand better how the edge case happens.

A different low effort idea is to edit the docstring to mention that "None" is return in some edge cases, so that developers are aware of this edge case.

[fujiapple852]

@engn33r my explanation above was not very clear, my apologies. Here is a minimal example to demonstrate WebSocketApp.run_forever() returning None:

import signal

import websocket

if __name__ == '__main__':
    def shutdown(*args):
        print("close")
        ws.close()


    signal.signal(signal.SIGINT, shutdown)
    ws = websocket.WebSocketApp("wss://example.com")
    teardown = ws.run_forever()
    print(teardown)

If you pass SIGINT to the python process (i.e. run kill -s INT <pid> or otherwise) the program will output close and then None.

The core issue here is the assumption that dispatcher.read(self.sock.sock, read, check) will only ever exit with an Exception, which is not true when the driving application is performing signal handling and explicitly closing the WebSocketApp with close(). In this case, as no exception occurs, the special handling for KeyboardInterrupt (i.e. SIGINT) does not occur and None is returned.

I've raised a PR (#788) to have run_forever() return False unconditionally in the event of a clean (no exception) exit and clarified the documentation of teardown.

The intended meaning of the name teardown is still a little unclear to me and it feels odd that returning False is the "happy path" case. However given this minimal change is (arguably) not a breaking change (given that it removes an undocumented edge case) I think it is lowest impact way of proceeding. I flagged it as a breaking change in the draft PR.