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

Support multiple readers in the asyncio hub #874

Open
Tracked by #868
itamarst opened this issue Jan 10, 2024 · 9 comments
Open
Tracked by #868

Support multiple readers in the asyncio hub #874

itamarst opened this issue Jan 10, 2024 · 9 comments

Comments

@itamarst
Copy link
Contributor

itamarst commented Jan 10, 2024

The asyncio hub in #869 doesn't support multiple readers on a single file descriptor.

The mechanism which implements this feature is very confusing to me; additional readers are added in a BaseHub.secondaries data structure, and then promoted to a primary listener if the main listener is removed for some reason. I don't understand how any of it works at all, and so I've failed to get it work.

@4383
Copy link
Member

4383 commented Mar 27, 2024

Apparently Openstack Swift relies at some point on this multiple readers mechanisms.
We observed related warnings in our attempts to switch on the Asyncio hub on devstack:

Mar 26 18:16:18.294113 np0037167578 systemd[1]: Started Devstack devstack@s-account.service.
Mar 26 18:16:19.156738 np0037167578 swift-account-server[72277]: Traceback (most recent call last):
Mar 26 18:16:19.156738 np0037167578 swift-account-server[72277]:   File "/opt/stack/data/venv/bin/swift-account-server", line 7, in <module>
Mar 26 18:16:19.157367 np0037167578 swift-account-server[72277]:     exec(compile(f.read(), __file__, 'exec'))
Mar 26 18:16:19.157367 np0037167578 swift-account-server[72277]:   File "/opt/stack/swift/bin/swift-account-server", line 23, in <module>
Mar 26 18:16:19.157367 np0037167578 swift-account-server[72277]:     sys.exit(run_wsgi(conf_file, 'account-server', **options))
Mar 26 18:16:19.157367 np0037167578 swift-account-server[72277]:   File "/opt/stack/swift/swift/common/wsgi.py", line 874, in run_wsgi
Mar 26 18:16:19.157367 np0037167578 swift-account-server[72277]:     conf, logger, global_conf, strategy = check_config(
Mar 26 18:16:19.157367 np0037167578 swift-account-server[72277]:   File "/opt/stack/swift/swift/common/wsgi.py", line 816, in check_config
Mar 26 18:16:19.157367 np0037167578 swift-account-server[72277]:     _initrp(conf_path, app_section, *args, **kwargs)
Mar 26 18:16:19.157367 np0037167578 swift-account-server[72277]:   File "/opt/stack/swift/swift/common/wsgi.py", line 1081, in _initrp
Mar 26 18:16:19.157822 np0037167578 swift-account-server[72277]:     logger = get_logger(conf, log_name,
Mar 26 18:16:19.157822 np0037167578 swift-account-server[72277]:   File "/opt/stack/swift/swift/common/utils/__init__.py", line 1951, in get_logger
Mar 26 18:16:19.158194 np0037167578 swift-account-server[72277]:     handler = ThreadSafeSysLogHandler(address=log_address,
Mar 26 18:16:19.158194 np0037167578 swift-account-server[72277]:   File "/usr/lib/python3.10/logging/handlers.py", line 861, in __init__
Mar 26 18:16:19.158194 np0037167578 swift-account-server[72277]:     logging.Handler.__init__(self)
Mar 26 18:16:19.158406 np0037167578 swift-account-server[72277]:   File "/usr/lib/python3.10/logging/__init__.py", line 884, in __init__
Mar 26 18:16:19.158573 np0037167578 swift-account-server[72277]:     self.createLock()
Mar 26 18:16:19.158573 np0037167578 swift-account-server[72277]:   File "/opt/stack/swift/swift/common/utils/__init__.py", line 6129, in createLock
Mar 26 18:16:19.159915 np0037167578 swift-account-server[72277]:     self.lock = NoopMutex()
Mar 26 18:16:19.159915 np0037167578 swift-account-server[72277]:   File "/opt/stack/swift/swift/common/utils/__init__.py", line 6116, in __init__
Mar 26 18:16:19.160925 np0037167578 swift-account-server[72277]:     eventlet.debug.hub_prevent_multiple_readers(False)
Mar 26 18:16:19.160925 np0037167578 swift-account-server[72277]:   File "/opt/stack/data/venv/lib/python3.10/site-packages/eventlet/debug.py", line 159, in hub_prevent_multiple_readers
Mar 26 18:16:19.161060 np0037167578 swift-account-server[72277]:     raise RuntimeError("Multiple readers are not yet supported by asyncio hub")
Mar 26 18:16:19.161060 np0037167578 swift-account-server[72277]: RuntimeError: Multiple readers are not yet supported by asyncio hub
Mar 26 18:16:19.161186 np0037167578 swift-account-server[72277]: Exception ignored in atexit callback: <function shutdown at 0x7f801837b910>
Mar 26 18:16:19.161186 np0037167578 swift-account-server[72277]: Traceback (most recent call last):
Mar 26 18:16:19.161186 np0037167578 swift-account-server[72277]:   File "/usr/lib/python3.10/logging/__init__.py", line 2191, in shutdown
Mar 26 18:16:19.161563 np0037167578 swift-account-server[72277]:     h.release()
Mar 26 18:16:19.161563 np0037167578 swift-account-server[72277]:   File "/usr/lib/python3.10/logging/__init__.py", line 923, in release
Mar 26 18:16:19.161816 np0037167578 swift-account-server[72277]:     if self.lock:
Mar 26 18:16:19.161816 np0037167578 swift-account-server[72277]: AttributeError: 'ThreadSafeSysLogHandler' object has no attribute 'lock'
Mar 26 18:16:19.430532 np0037167578 systemd[1]: devstack@s-account.service: Main process exited, code=exited, status=1/FAILURE
Mar 26 18:16:19.430556 np0037167578 systemd[1]: devstack@s-account.service: Failed with result 'exit-code'.

For more context, please see:

So if we want to see Swift transitioned to Asyncio, either we have to introduce support for this feature on the Asyncio hub, or Swift should drop usages of this feature before starting its migration.

@4383
Copy link
Member

4383 commented Mar 27, 2024

Toogling hub_prevent_multiple_readers to True should remove this problem, however, we need to see if swift really needs to disable this prevent feature.

def hub_prevent_multiple_readers(state=True):

@4383
Copy link
Member

4383 commented Mar 27, 2024

From swift's code:

    def __init__(self):
        # Usually, it's an error to have multiple greenthreads all waiting
        # to write to the same file descriptor. It's often a sign of inadequate
        # concurrency control; for example, if you have two greenthreads
        # trying to use the same memcache connection, they'll end up writing
        # interleaved garbage to the socket or stealing part of each others'
        # responses.
        #
        # In this case, we have multiple greenthreads waiting on the same
        # (logging) file descriptor by design. So, similar to the PipeMutex,
        # we must turn off eventlet's multiple-waiter detection.
        #
        # It would be better to turn off multiple-reader detection for only
        # the logging socket fd, but eventlet does not support that.
        eventlet.debug.hub_prevent_multiple_readers(False)

https://opendev.org/openstack/swift/src/branch/master/swift/common/utils/__init__.py#L6102-L6116

@4383
Copy link
Member

4383 commented Mar 27, 2024

In all the case, the Swift use case is based on an Eventlet debug feature which is used to disable protection at runtime...

https://eventlet.readthedocs.io/en/latest/modules/debug.html#eventlet.debug.hub_prevent_multiple_readers

IMO this "enabling" feature should be only used for debug purpose, and not to be used to allow race conditions between greenthreads by bypassing protection.

@4383
Copy link
Member

4383 commented Mar 27, 2024

Added multiple-reader prevention code because it seems to be a fairly common pitfall

From:

cb7c8c0

@4383
Copy link
Member

4383 commented Mar 27, 2024

               raise RuntimeError("Second simultaneous %s on fileno %s "\
                     "detected.  Unless you really know what you're doing, "\
                     "make sure that only one greenthread can %s any "\
                     "particular socket.  Consider using a pools.Pool. "\
                     "If you do know what you're doing and want to disable "\
                     "this error, call "\
                     "eventlet.debug.hub_multiple_reader_prevention(False)" % (
                     evtype, fileno, evtype))

Still from cb7c8c0

So definitely, I don't think we should rely on this feature at all.
I don't even think we should implement it on the Asyncio hub.

Concurrent accesses to resources and possible race conditions should be handled on user side.
IMO Users should design their code by taking account of resources access and lock things that needs to be protected from concurrent access (fd, socket, etc)

@itamarst
Copy link
Contributor Author

I suspect most networking frameworks don't allow this sort of feature, yes, it doesn't really make sense for the vast majority of users (and for those who want to do it, there's a workaround, I'm pretty sure: dup() the file descriptor).

@4383
Copy link
Member

4383 commented Mar 27, 2024

I'd suggest to close this github issue, and maybe to add a special note on the docs (maybe in the migration guide) to be transparent about the multiple reader feature and its compatibility with the Asyncio hub. We could summarize our point and our arguments in the docs.

@4383
Copy link
Member

4383 commented Mar 28, 2024

As this "mutliple reader" functionality seems heavily used (see the list of commits which refer to this feature in #432), I'd also suggest to amend our eventlet documentation with a dedicated section to give advices on how to refactor migrated code, to bypass "multiple readers".

IMO it should be fixed by design at the user app level.

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

No branches or pull requests

2 participants