Resolves #80

[cjrh]

Description

Check before calling set_result() or set_exception() to see whether the future is already done() or not.

Fixes: #80

[cjrh]

It seems that as a consequence of these changes, the test_multi_loop_usage test appears to hang:

$ python -m coverage run -m aiosqlite.tests
test_backup_aiosqlite (aiosqlite.tests.smoke.SmokeTest) ... 22:07:41,82 I core:run():105   returning exception no such table: foo
101 0 2
ok
test_backup_py36 (aiosqlite.tests.smoke.SmokeTest) ... skipped 'Test short circuit fail on Py 3.6'
test_backup_sqlite (aiosqlite.tests.smoke.SmokeTest) ... ok
test_connect_error (aiosqlite.tests.smoke.SmokeTest) ... 22:07:41,91 I core:run():105   returning exception unable to open database fi
le
22:07:41,92 I core:run():105   returning exception unable to open database file
ok
test_connection_await (aiosqlite.tests.smoke.SmokeTest) ... ok
test_connection_context (aiosqlite.tests.smoke.SmokeTest) ... ok
test_connection_locations (aiosqlite.tests.smoke.SmokeTest) ... ok
test_connection_properties (aiosqlite.tests.smoke.SmokeTest) ... ok
test_context_cursor (aiosqlite.tests.smoke.SmokeTest) ... ok
test_create_function (aiosqlite.tests.smoke.SmokeTest)
Assert that after creating a custom function, it can be used ... ok
test_create_function_deterministic_post38 (aiosqlite.tests.smoke.SmokeTest)
Assert that after creating a deterministic custom function, it can be used. ... ok
test_create_function_deterministic_pre38 (aiosqlite.tests.smoke.SmokeTest)
Make sure the deterministic parameter cannot be used in old Python versions ... skipped 'Python < 3.8 specific behaviour'
test_cursor_return_self (aiosqlite.tests.smoke.SmokeTest) ... ok
test_enable_load_extension (aiosqlite.tests.smoke.SmokeTest)
Assert that after enabling extension loading, they can be loaded ... 22:07:41,239 I core:run():105   returning exception test.so: cann
ot open shared object file: No such file or directory
ok
test_fetch_all (aiosqlite.tests.smoke.SmokeTest) ... ok
test_iterable_cursor (aiosqlite.tests.smoke.SmokeTest) ... ok
test_iterdump (aiosqlite.tests.smoke.SmokeTest) ... ok
test_multi_loop_usage (aiosqlite.tests.smoke.SmokeTest) ... 
<hangs here>

I'm not yet sure why.

[amyreese]

I'd like to see a test cases that triggers the InvalidStateError that this is supposed to be fixing, as well as some understanding on why the multi-loop tests hang with this change. Would it be better to instead wrap the set_result/set_exception calls in a try/except clause to catch the InvalidStateError? I suspect there's still "technically" a race condition even with the current check if the thread setting the result gets swapped out between the if future.done() check and the call to set_result.

[cjrh]

@jreese Thanks for taking a look at it.

Honestly, I only submitted this to demonstrate in #74 that CTRL-C can be handled adequately (in conjunction with the interrupt api). I don't need this functionality, and I don't have spare bandwidth in the short term to move this along so anybody else is more than welcome to take ownership of this PR.

[cjrh]

I suspect there's still "technically" a race condition even with the current check if the thread setting the result gets swapped out between the if future.done() check and the call to set_result

I considered this, and I think it's safe. The callback given to call_soon_threadsafe will run in the same thread as where the future is getting cancelled so as I understand it, cancellation cannot preempt between .done() and the .set_result(). (This is with custom signal handlers. I can't remember exactly how KeyboardInterrupt behaves). Would you agree or have I missed something?

[amyreese]

That's a good point. Makes me wonder even more why that causes multi-loop tests to hang. 🤔

[cjrh]

I figured it out, it was a standard garden-variety closure bug, on the future identifier.

[amyreese]

Thank you for following up on that!