CabbageDevelopment / qasync

Python library for using asyncio in Qt-based applications.
BSD 2-Clause "Simplified" License
307 stars 43 forks source link

_IocpProactor: Acquire lock in previously missed methods #87

Closed dnadlinger closed 9 months ago

dnadlinger commented 9 months ago

For the same reason that recv(), send(), accept() and connect() need to acquire the lock, all the other operations need to as well.

This also shows a design limitation in this approach w.r.t. maintainability (silent failure when new operations are added); it would be preferable to either reproduce the windows_events implementation in its entirety, or use proxying via composition to ensure methods not made thread-safe cannot be invoked.

Finally, the _poll implementation seems to have further issues: it is not clear to me that the _stopped_serving handling is thread-safe, _unregistered is not handled, and it seems preferable not to hold the lock when calling GetQueuedCompletionStatus. I restricted this pull request to addressing the straight-forward omission of wrappers for ease of review, however.

hosaka commented 9 months ago

Thanks for the PR. It makes sense to wrap the missing methods with a mutex, happy to merge this.

On the _poll implementation, from my understanding the idiom WeakRef is None should be thread safe. Whether that implies that obj in WeakSet is safe or not, I am not sure. Looking at weakref docs:

# r is a weak reference object
o = r()
if o is None:
    # referent has been garbage collected
    print("Object has been deallocated; can't frobnicate.")
else:
    print("Object is still live!")
    o.do_something_useful()

Using a separate test for “liveness” creates race conditions in threaded applications; another thread can cause a weak reference to become invalidated before the weak reference is called; the idiom shown above is safe in threaded applications as well as single-threaded applications.

The WeakSet.__iter__ itself uses and _IterationGuard which has a comment about it being "relatively" thread-safe, however this SO post suggests otherwise.