search

Tribler / tribler

Privacy enhanced BitTorrent client with P2P content discovery
https://www.tribler.org
GNU General Public License v3.0
4.85k stars 451 forks source link

Better defensive programming required for finishing Twisted requests #4912

Closed DanGraur closed 5 years ago

DanGraur commented 5 years ago

As discussed in PR #4910 (which stemmed from issue #4909), using the condition if not request.finished: (where request is of the twisted.web.http.Request type) as a guard prior to calling request.finish(), does not always prove to be safe and sufficient. In certain scenarios, such as when the client abruptly terminates the connection, the request.finished field is not set to True, and the request.finish() method will be called in unsafe circumstances, triggering an error of the following type:

Traceback (most recent call last):
  File "/data/projects/python/tribler/TriblerGUI/event_request_manager.py", line 114, in on_read_data
    raise RuntimeError(json_dict["event"]["text"])
RuntimeError: [Failure instance: Traceback: <type 'exceptions.RuntimeError'>: Request.finish called on a request after its connection was lost; use Request.notifyFinish to keep track of this.
/home/dan/.local/lib/python2.7/site-packages/twisted/internet/defer.py:568:_startRunCallbacks
/home/dan/.local/lib/python2.7/site-packages/twisted/internet/defer.py:654:_runCallbacks
/home/dan/.local/lib/python2.7/site-packages/twisted/internet/defer.py:460:callback
/home/dan/.local/lib/python2.7/site-packages/twisted/internet/defer.py:568:_startRunCallbacks
--- <exception caught here> ---
/home/dan/.local/lib/python2.7/site-packages/twisted/internet/defer.py:654:_runCallbacks
/data/projects/python/tribler/Tribler/Core/Modules/restapi/mychannel_endpoint.py:306:_on_add_failed
/home/dan/.local/lib/python2.7/site-packages/twisted/python/failure.py:462:trap
/home/dan/.local/lib/python2.7/site-packages/twisted/internet/defer.py:654:_runCallbacks
/data/projects/python/tribler/Tribler/Core/Modules/restapi/mychannel_endpoint.py:303:_on_added
/home/dan/.local/lib/python2.7/site-packages/twisted/web/server.py:268:finish
/home/dan/.local/lib/python2.7/site-packages/twisted/web/http.py:1071:finish
]

In such scenarios, Twisted will update the request._disconnected protected field, by setting it to True. Hence, one solution to this issue is to update the aforementioned if clauses from if not request.finished: to if not request.finished and not request._disconnected:, in order to improve the robustness of Tribler in such scenarios.

ichorid commented 5 years ago

As it was said earlier, we are on the verge of ditching Twisted in favour of asyncio+AIOHTTP. The endpoints protocol is quite different there.