Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

avoid cancel_handle close race with weakref.finalize #2374

Closed
wants to merge 3 commits into from

Conversation

richardsheridan
Copy link
Contributor

@richardsheridan richardsheridan commented Jul 7, 2022

Fixes #1271. This is essentially the reference counting scheme from that issue, but python is doing the refcounting. I've also made sure the "nonblocking" case is a full checkpoint, win32 errors don't pass silently, and the logic is protected from KeyboardInterrupt.

I wrote this in response to #1834 (comment) where @honglei seemed more interested in the bugfix than the features of #1834. (Hopefully you can test this branch in your application?)

Ideally, if someone reviews this they would also have a look at #1834 so I can either close or merge that one.

@codecov
Copy link

codecov bot commented Jul 7, 2022

Codecov Report

Merging #2374 (2e9d220) into master (b24672a) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2374      +/-   ##
==========================================
- Coverage   92.45%   92.42%   -0.04%     
==========================================
  Files         118      118              
  Lines       16342    16348       +6     
  Branches     3155     3156       +1     
==========================================
  Hits        15109    15109              
- Misses       1104     1110       +6     
  Partials      129      129              
Impacted Files Coverage Δ
trio/_core/_io_windows.py 0.00% <ø> (ø)
trio/_core/_windows_cffi.py 0.00% <0.00%> (ø)
trio/_wait_for_object.py 0.00% <0.00%> (ø)
trio/tests/test_wait_for_object.py 10.29% <0.00%> (-0.08%) ⬇️

@honglei
Copy link

honglei commented Jul 9, 2022

at the beginning I wanna make pynorm https://github.com/USNavalResearchLaboratory/norm work with asyncio and fastapi, so I need an async version of WaitForSingleObject , then I found trio.
But trio is not asyncio, #892 ,
so I need some time to learn/understand how the trio makes things work.

import pynorm
instance:pynorm.Instance = pynorm.Instance()
async def watch_norm_events():
    '''
    NormGetNextEvent docs : non-blocking operation may be achieved by using the NormGetDescriptor() function
    to obtain a descriptor(int for Unix or HANDLE for WIN32) suitable for 
    asynchronous input/output (I/O) notification
    using such system calls the Unix select() or Win32 WaitForMultipleObjects() calls. 
    The descriptor is signaled when a notification event is pending and a call to 
    NormGetNextEvent() will not block
    '''
    global instance
    #instance.setDebugLevel(level=12)

    handle = instance.getDescriptor() #handler opened by norm lib
    while True:
        await WaitForSingleObject(handle)  # wait for event 
        event: pynorm.event.Event = instance.getNextEvent( )
        print(event) 

@richardsheridan richardsheridan changed the title avoid cancel_handle close race with custom thread dispatch avoid cancel_handle close race with weakref.finalize Mar 4, 2023
@richardsheridan
Copy link
Contributor Author

This PR has 100% diff branch coverage on my machine.

try:
await trio.to_thread.run_sync(
WaitForMultipleObjects_sync,
handle,
cancel_handle,
cancellable=True,
limiter=trio.CapacityLimiter(math.inf),
limiter=trio.CapacityLimiter(1),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for this change? The two versions should act the same, given that we're creating a new CapacityLimiter on every call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing super important, but it avoids an import and an attribute lookup. Do you remember why the default thread limiter is not used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just answering my own question here: #4 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Race condition in trio.hazmat.WaitForSingleObject
3 participants