Skip to content

Commit

Permalink
Fix mysterious python 2 crash (#87)
Browse files Browse the repository at this point in the history
- fix mysterious python 2 crash
- add regression test for mysterious python 2 crash
- fix actual mis-use of PyWeakref_GetObject()
- bump version#
  • Loading branch information
graebm authored Oct 17, 2019
1 parent 3da3a7b commit 0ffa99c
Show file tree
Hide file tree
Showing 5 changed files with 38 additions and 3 deletions.
2 changes: 1 addition & 1 deletion setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ def awscrt_ext():

setuptools.setup(
name="awscrt",
version="0.3.1",
version="0.3.2",
author="Amazon Web Services, Inc",
author_email="[email protected]",
description="A common runtime for AWS Python projects",
Expand Down
3 changes: 2 additions & 1 deletion source/http_message.c
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,8 @@ static bool s_http_message_update_from_py(struct http_message_binding *message)

/* Doing a lot of queries so grab a hard reference */
message_self = PyWeakref_GetObject(message->self_proxy);
if (!message_self) {
if (message_self == Py_None) {
PyErr_SetString(PyExc_RuntimeError, "HttpMessageBase destroyed");
goto done;
}
Py_INCREF(message_self);
Expand Down
7 changes: 6 additions & 1 deletion source/http_stream.c
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,12 @@ static void s_on_stream_complete(struct aws_http_stream *native_stream, int erro
Py_DECREF(result);

/* DECREF python self, we don't need to force it to stay alive any longer. */
Py_DECREF(PyWeakref_GetObject(stream->self_proxy));
PyObject *self = PyWeakref_GetObject(stream->self_proxy);
Py_DECREF(self);
/* Hack note. This use to be a one liner: Py_DECREF(PyWeakref_GetObject(stream->self_proxy));
* that would crash on Python 2, saying we were DECREF'ing Py_None.
* That should be impossible because we're forcing the python self to stay alive.
* I have no idea why splitting it into 2 lines fixes everything, but it does. */

PyGILState_Release(state);
/*************** GIL RELEASE ***************/
Expand Down
5 changes: 5 additions & 0 deletions test/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import gc
import inspect
import sys
import time
import types
import unittest

Expand All @@ -31,6 +32,10 @@ def setUp(self):
def tearDown(self):
gc.collect()

# Native resources might need a few more ticks to finish cleaning themselves up.
if NativeResource._living:
time.sleep(1)

# Print out debugging info on leaking resources
if NativeResource._living:
print('Leaking NativeResources:')
Expand Down
24 changes: 24 additions & 0 deletions test/test_http_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,30 @@ def test_put_http(self):
def test_put_https(self):
self._test_put(secure=True)

# Ensure that stream and connection classes stay alive until work is complete
def _test_stream_lives_until_complete(self, secure):
self._start_server(secure)
connection = self._new_client_connection(secure)

request = HttpRequest('GET', '/test/test_http_client.py')
stream = connection.request(request)
completion_future = stream.completion_future

# delete all local references
del stream
del connection

# stream should still complete successfully
completion_future.result(self.timeout)

self._stop_server()

def test_stream_lives_until_complete_http(self):
self._test_stream_lives_until_complete(secure=False)

def test_stream_lives_until_complete_https(self):
self._test_stream_lives_until_complete(secure=True)


if __name__ == '__main__':
unittest.main()

0 comments on commit 0ffa99c

Please sign in to comment.