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

Jump to bottom

asyncio proactor udp transport stops responding after send to port that isn't listening #91227

Open
esoma mannequin opened this issue Mar 20, 2022 · 8 comments
Open

asyncio proactor udp transport stops responding after send to port that isn't listening #91227

esoma mannequin opened this issue Mar 20, 2022 · 8 comments
Labels
3.10 expert-asyncio OS-windows

Comments

@esoma
Copy link
Mannequin

esoma mannequin commented Mar 20, 2022

BPO 47071
Nosy @pfmoore, @tjguk, @asvetlov, @zware, @1st1, @zooba, @esoma
PRs
  • bpo-47071: Ignore ERROR_PORT_UNREACHABLE. #32011
    		•
    Files
  • udpbug2.py
  • windows_events.py
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
closed_at = None
created_at = <Date 2022-03-20.14:58:17.671>
labels = ['3.10', 'OS-windows', 'expert-asyncio']
title = "asyncio proactor udp transport stops responding after send to port that isn't listening"
updated_at = <Date 2022-03-20.15:48:54.679>
user = 'https://github.com/esoma'

    bugs.python.org fields:

    activity = <Date 2022-03-20.15:48:54.679>
actor = 'esoma'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Windows', 'asyncio']
creation = <Date 2022-03-20.14:58:17.671>
creator = 'esoma'
dependencies = []
files = ['50691', '50692']
hgrepos = []
issue_num = 47071
keywords = ['patch']
message_count = 4.0
messages = ['415611', '415612', '415614', '415617']
nosy_count = 7.0
nosy_names = ['paul.moore', 'tim.golden', 'asvetlov', 'zach.ware', 'yselivanov', 'steve.dower', 'esoma']
pr_nums = ['32011']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = None
url = 'https://bugs.python.org/issue47071'
versions = ['Python 3.10']
    @esoma
    Copy link
    Mannequin Author

    esoma mannequin commented Mar 20, 2022

    Reproducer attached. Change USE_PROACTOR to False to use the SelectorEventLoop instead, which doesn't exhibit this behavior.

    The output on my machine when using the proactor loop is:

    datagram received b'ping 1'
datagram received b'ping 2'

    And the selector loop (which is the behavior I would expect):

    datagram received b'ping 1'
datagram received b'ping 2'
datagram received b'ping 3'

    At a high level, after sending data to an address that isn't listening the asyncio protocol will no longer receive messages.

    Digging deeper, _ProactorDatagramTransport._loop_reading encounters the windows error 1234 (ERROR_PORT_UNREACHABLE) after the "bad send". It appears this a (undocumented/buggy?) behavior of WSARecvFrom where the next call to it after an unreachable WSASendTo in UDP mode will return the ICMP unreachable message. The actual error is returned from GetOverlappedResult.

    I've hacked together a fix that retries IocpProactor.recvfrom if the result is ERROR_PORT_UNREACHABLE. It fixes the issue for the reproducer and my actual use case, but it's probably not ideal. My solution for the moment is just to use the SelectorEventLoop instead.

    @esoma
    Copy link
    Mannequin Author

    esoma mannequin commented Mar 20, 2022

    Uploading my hack to asyncio.windows_events.py -- this is based off 3.10.2's distribution.

    @asvetlov
    Copy link
    Contributor

    asvetlov commented Mar 20, 2022

    Thanks for the report.
    Could you make a pull request?
    The whole attached windows_events.py is hard to analyze.

    @esoma
    Copy link
    Mannequin Author

    esoma mannequin commented Mar 20, 2022

    Certainly: #32011

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @esoma esoma mentioned this issue Jul 6, 2022
    Open
    @msoxzw
    Copy link
    Contributor

    msoxzw commented Jul 26, 2022

    As far as I know, there are several alternative solutions from open source projects:
    The Go programming language - https://github.com/golang/go/blob/ceefd3a37bb2a77b1e9a76fc74360c68ef16ddb8/src/internal/poll/fd_windows.go#L341-L352
    The libuv - https://github.com/libuv/libuv/blob/6c692ad1cbcc5083ec90954a4b091b660aedfc10/src/win/udp.c#L462-L486

    @esoma
    Copy link
    Sponsor Contributor

    esoma commented Jul 26, 2022

    Looks like the libuv solution is similar to my PR -- just trying again.

    The go solution is much simpler though. I'll try that out.

    @msoxzw
    Copy link
    Contributor

    msoxzw commented Jul 26, 2022

    The libuv ignores WSAECONNRESET at lower level, so it is unnecessary to try again.

    @esoma
    Copy link
    Sponsor Contributor

    esoma commented Jul 28, 2022

    Ah, yeah I see. iirc, that was something I tried initially, but I was concerned about whether that would have impact elsewhere since Overlapped.getresult is also used by the connection based protocols where ERROR_PORT_UNREACHABLE probably should not be ignored. I think I was also getting some undesirable behavior, but I can't reproduce it so maybe I was just doing something weird. I suppose if it is an issue that can be taken care of with some internal state to the Overlapped object that tracks whether the last WSA command was WSARecvFrom or not and ignoring conditionally based on that. For the record, I've tested and ignoring it fixes the reproducer.

    I thought about the Go approach a bit more I'm not sure where the appropriate place for this would even be. It seems inappropriate to change this behavior in the socket extension or for _overlapped to silently change this aspect an externally created socket.

    I've also went back and tried just ignoring the error in window_events.IocpProactor.recvfrom.<locals>.finish_recv, since we can specifically target WSARecvFrom results that way, and it works just fine. I'm not sure why I thought it needed to be so complicated. There already exists a convention for returning no data to indicate "ignore this and keep going". I'll update my PR to use this method.

    @neonene neonene mentioned this issue Jul 29, 2022
    Closed
    @beauxq beauxq mentioned this issue Oct 16, 2022
    Merged
    beauxq pushed a commit to beauxq/Archipelago that referenced this issue Oct 17, 2022
    workaround for asyncio udp bug
    8aa6baa 
    There is a bug in Python in Windows
python/cpython#91227
that makes it so if I look for RetroArch before it's ready, it breaks the asyncio udp transport system.

As a workaround, we don't look for RetroArch until the user asks for it with /sms
    black-sliver pushed a commit to ArchipelagoMW/Archipelago that referenced this issue Oct 20, 2022
    @beauxq @SoldierofOrder
    New Game: Zillion (#1081)
    265ee70 
    * Option RangeWithSpecialMax

* amendment to typing in web options

* compare string with number

* lots of work on zillion

* fix zillion fill logic

* fix a few more issues in zillion fill logic

* can make zillion patch and use it

* put multi items in zillion rom

* work on ZillionClient

* logging and auth in client

* work on sending and receiving items

* implement item_handling flag

* fix locations ids to NuktiServer package

* use rewrite of zri

* cache logic rule data for performance

* use new id maps

* fix some problems with the big recent merge

* ZillionClient: use new context manager for Memory class

* fix ItemClassification for Zillion items
and some debug statements for asserts,
documentation on running scripts for manual testing
type correction in CommonContext

* fix some issues in client, start on docs, put rescue and item ram addresses in slot data

* use new location name system
fix item locations getting out of sync in progression balancing

* zillion client can read slot name from game

* zillion: new item names

* remove extra unneeded import

* newer options (room gen and starting cards)

* update comment in zillion patch

* zillion non static regions

* change some logging, update some comments

* allow ZillionClient to exit in certain situations

* todo note to fix options doc strings

* don't force auto forfeit

* rework validation of floppy requirement and item counts
and fix race condition in generate_output

* reorganize Zillion component structure
with System class

* documentation updates for Zillion

* attempt inno_setup.iss

* remove todo comment for something done

* update comment

* rework item count zillion options
and some small cleanups

* fix location check count

* data package version 1

* Zillion can pass unit tests without rom

* fix freeze if closing ZillionClient while it's waiting for server login

* specify commit hash for zilliandomizer package

* some changes to options validation

* Zillion doors saved on multiworld server

* add missing function in inno_setup
and name of vanilla continues in options

* rework zillion sync task and context

* Apply documentation suggestions from SoldierofOrder

Co-authored-by: SoldierofOrder <107806872+SoldierofOrder@users.noreply.github.com>

* update zillion package

* workaround for asyncio udp bug

There is a bug in Python in Windows
python/cpython#91227
that makes it so if I look for RetroArch before it's ready, it breaks the asyncio udp transport system.

As a workaround, we don't look for RetroArch until the user asks for it with /sms

* a few of the smaller suggestions from review

* logic only looks at my locations
instead of all the multiworld locations

* some adjustments from pull request discussion
and some unit tests

* patch webhost changes from pull request discussion

* zillion logic tests

* better vblr test

* test interaction of character rescue items with logic

* move unit tests to new worlds folder

* comment improvements

* fix minor logic issue
and add memory read timeout

* capitalization in option display names
Opa-Opa is a proper noun

* redirect zz stdout to debug

* fix option validation bug making unbeatable seeds

* remove line that does nothing

* attach logic cache to world

Co-authored-by: SoldierofOrder <107806872+SoldierofOrder@users.noreply.github.com>
Co-authored-by: Doug Hoskisson <doughoskisson@novuslabs.com>
    LiquidCat64 pushed a commit to LiquidCat64/LiquidCatipelago that referenced this issue Oct 22, 2022
    @beauxq @SoldierofOrder @LiquidCat64
    New Game: Zillion (ArchipelagoMW#1081)
    3397b46 
    * Option RangeWithSpecialMax

* amendment to typing in web options

* compare string with number

* lots of work on zillion

* fix zillion fill logic

* fix a few more issues in zillion fill logic

* can make zillion patch and use it

* put multi items in zillion rom

* work on ZillionClient

* logging and auth in client

* work on sending and receiving items

* implement item_handling flag

* fix locations ids to NuktiServer package

* use rewrite of zri

* cache logic rule data for performance

* use new id maps

* fix some problems with the big recent merge

* ZillionClient: use new context manager for Memory class

* fix ItemClassification for Zillion items
and some debug statements for asserts,
documentation on running scripts for manual testing
type correction in CommonContext

* fix some issues in client, start on docs, put rescue and item ram addresses in slot data

* use new location name system
fix item locations getting out of sync in progression balancing

* zillion client can read slot name from game

* zillion: new item names

* remove extra unneeded import

* newer options (room gen and starting cards)

* update comment in zillion patch

* zillion non static regions

* change some logging, update some comments

* allow ZillionClient to exit in certain situations

* todo note to fix options doc strings

* don't force auto forfeit

* rework validation of floppy requirement and item counts
and fix race condition in generate_output

* reorganize Zillion component structure
with System class

* documentation updates for Zillion

* attempt inno_setup.iss

* remove todo comment for something done

* update comment

* rework item count zillion options
and some small cleanups

* fix location check count

* data package version 1

* Zillion can pass unit tests without rom

* fix freeze if closing ZillionClient while it's waiting for server login

* specify commit hash for zilliandomizer package

* some changes to options validation

* Zillion doors saved on multiworld server

* add missing function in inno_setup
and name of vanilla continues in options

* rework zillion sync task and context

* Apply documentation suggestions from SoldierofOrder

Co-authored-by: SoldierofOrder <107806872+SoldierofOrder@users.noreply.github.com>

* update zillion package

* workaround for asyncio udp bug

There is a bug in Python in Windows
python/cpython#91227
that makes it so if I look for RetroArch before it's ready, it breaks the asyncio udp transport system.

As a workaround, we don't look for RetroArch until the user asks for it with /sms

* a few of the smaller suggestions from review

* logic only looks at my locations
instead of all the multiworld locations

* some adjustments from pull request discussion
and some unit tests

* patch webhost changes from pull request discussion

* zillion logic tests

* better vblr test

* test interaction of character rescue items with logic

* move unit tests to new worlds folder

* comment improvements

* fix minor logic issue
and add memory read timeout

* capitalization in option display names
Opa-Opa is a proper noun

* redirect zz stdout to debug

* fix option validation bug making unbeatable seeds

* remove line that does nothing

* attach logic cache to world

Co-authored-by: SoldierofOrder <107806872+SoldierofOrder@users.noreply.github.com>
Co-authored-by: Doug Hoskisson <doughoskisson@novuslabs.com>
    CaitSith2 added a commit to ArchipelagoMW/Archipelago that referenced this issue Oct 27, 2022
    @beauxq @SoldierofOrder @CaitSith2
    Zillion: map tracker in client (#1136)
    aeb78ea 
    * Option RangeWithSpecialMax

* amendment to typing in web options

* compare string with number

* lots of work on zillion

* fix zillion fill logic

* fix a few more issues in zillion fill logic

* can make zillion patch and use it

* put multi items in zillion rom

* work on ZillionClient

* logging and auth in client

* work on sending and receiving items

* implement item_handling flag

* fix locations ids to NuktiServer package

* use rewrite of zri

* cache logic rule data for performance

* use new id maps

* fix some problems with the big recent merge

* ZillionClient: use new context manager for Memory class

* fix ItemClassification for Zillion items
and some debug statements for asserts,
documentation on running scripts for manual testing
type correction in CommonContext

* fix some issues in client, start on docs, put rescue and item ram addresses in slot data

* use new location name system
fix item locations getting out of sync in progression balancing

* zillion client can read slot name from game

* zillion: new item names

* remove extra unneeded import

* newer options (room gen and starting cards)

* update comment in zillion patch

* zillion non static regions

* change some logging, update some comments

* allow ZillionClient to exit in certain situations

* todo note to fix options doc strings

* don't force auto forfeit

* rework validation of floppy requirement and item counts
and fix race condition in generate_output

* reorganize Zillion component structure
with System class

* documentation updates for Zillion

* attempt inno_setup.iss

* remove todo comment for something done

* update comment

* rework item count zillion options
and some small cleanups

* fix location check count

* data package version 1

* Zillion can pass unit tests without rom

* fix freeze if closing ZillionClient while it's waiting for server login

* specify commit hash for zilliandomizer package

* some changes to options validation

* Zillion doors saved on multiworld server

* add missing function in inno_setup
and name of vanilla continues in options

* rework zillion sync task and context

* Apply documentation suggestions from SoldierofOrder

Co-authored-by: SoldierofOrder <107806872+SoldierofOrder@users.noreply.github.com>

* update zillion package

* workaround for asyncio udp bug

There is a bug in Python in Windows
python/cpython#91227
that makes it so if I look for RetroArch before it's ready, it breaks the asyncio udp transport system.

As a workaround, we don't look for RetroArch until the user asks for it with /sms

* a few of the smaller suggestions from review

* logic only looks at my locations
instead of all the multiworld locations

* some adjustments from pull request discussion
and some unit tests

* patch webhost changes from pull request discussion

* zillion logic tests

* better vblr test

* test interaction of character rescue items with logic

* move unit tests to new worlds folder

* comment improvements

* fix minor logic issue
and add memory read timeout

* capitalization in option display names
Opa-Opa is a proper noun

* client toggle side panel with /map

* displays map

* fix map transparency

* fix broken launcher

* better way to specify grid container

* start kivy typing

* have a map that updates with item checks

but it breaks other parts of the UI

* fix layout bug

* aspect ratio of image
and some type checking details

* Fix loading of map for compiled builds

Co-authored-by: SoldierofOrder <107806872+SoldierofOrder@users.noreply.github.com>
Co-authored-by: Doug Hoskisson <doughoskisson@novuslabs.com>
Co-authored-by: CaitSith2 <d_good@caitsith2.com>
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    3.10 expert-asyncio OS-windows
    Projects
    asyncio
    Status: Todo
    Milestone
    No milestone
    Development

    No branches or pull requests
    3 participants
    @asvetlov @esoma @msoxzw