-
-
Notifications
You must be signed in to change notification settings - Fork 99
Drop locks from the cheroot.ssl.pyopenssl.SSLConnection method
#787
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -52,7 +52,6 @@ | |||
|
|
||||
| import socket | ||||
| import sys | ||||
| import threading | ||||
| import time | ||||
| from warnings import warn as _warn | ||||
|
|
||||
|
|
@@ -176,99 +175,51 @@ class SSLFileobjectStreamWriter(SSLFileobjectMixin, StreamWriter): | |||
| """SSL file object attached to a socket object.""" | ||||
|
|
||||
|
|
||||
| class SSLConnectionProxyMeta: | ||||
| """Metaclass for generating a bunch of proxy methods.""" | ||||
|
|
||||
| def __new__(mcl, name, bases, nmspc): | ||||
| """Attach a list of proxy methods to a new class.""" | ||||
| proxy_methods = ( | ||||
| 'get_context', | ||||
| 'pending', | ||||
| 'send', | ||||
| 'write', | ||||
| 'recv', | ||||
| 'read', | ||||
| 'renegotiate', | ||||
| 'bind', | ||||
| 'listen', | ||||
| 'connect', | ||||
| 'accept', | ||||
| 'setblocking', | ||||
| 'fileno', | ||||
| 'close', | ||||
| 'get_cipher_list', | ||||
| 'getpeername', | ||||
| 'getsockname', | ||||
| 'getsockopt', | ||||
| 'setsockopt', | ||||
| 'makefile', | ||||
| 'get_app_data', | ||||
| 'set_app_data', | ||||
| 'state_string', | ||||
| 'sock_shutdown', | ||||
| 'get_peer_certificate', | ||||
| 'want_read', | ||||
| 'want_write', | ||||
| 'set_connect_state', | ||||
| 'set_accept_state', | ||||
| 'connect_ex', | ||||
| 'sendall', | ||||
| 'settimeout', | ||||
| 'gettimeout', | ||||
| 'shutdown', | ||||
| ) | ||||
| proxy_methods_no_args = ('shutdown',) | ||||
|
|
||||
| proxy_props = ('family',) | ||||
|
|
||||
| def lock_decorator(method): | ||||
| """Create a proxy method for a new class.""" | ||||
|
|
||||
| def proxy_wrapper(self, *args): | ||||
| self._lock.acquire() | ||||
| try: | ||||
| new_args = ( | ||||
| args[:] if method not in proxy_methods_no_args else [] | ||||
| ) | ||||
| return getattr(self._ssl_conn, method)(*new_args) | ||||
| finally: | ||||
| self._lock.release() | ||||
|
|
||||
| return proxy_wrapper | ||||
|
|
||||
| for m in proxy_methods: | ||||
| nmspc[m] = lock_decorator(m) | ||||
| nmspc[m].__name__ = m | ||||
|
|
||||
| def make_property(property_): | ||||
| """Create a proxy method for a new class.""" | ||||
|
|
||||
| def proxy_prop_wrapper(self): | ||||
| return getattr(self._ssl_conn, property_) | ||||
| class SSLConnection(SSL.Connection): | ||||
| """ | ||||
| A compatibility wrapper around :py:class:`OpenSSL.SSL.Connection`. | ||||
|
|
||||
| proxy_prop_wrapper.__name__ = property_ | ||||
| return property(proxy_prop_wrapper) | ||||
| This class exists primarily to ensure the standard Python socket method | ||||
| :py:meth:`.shutdown()` is available for interface compatibility. | ||||
| """ | ||||
|
|
||||
| for p in proxy_props: | ||||
| nmspc[p] = make_property(p) | ||||
| def makefile(self, *args, **kwargs): | ||||
| """ | ||||
| Raise :exc:`NotImplementedError` for ``makefile()`` interface. | ||||
|
|
||||
| # Doesn't work via super() for some reason. | ||||
| # Falling back to type() instead: | ||||
| return type(name, bases, nmspc) | ||||
| PyOpenSSL's :py:class:`~OpenSSL.SSL.Connection` marks | ||||
| :py:meth:`~OpenSSL.SSL.Connection.makefile` as not implemented. | ||||
| This method exists only for interface compatibility with | ||||
| Python sockets. | ||||
| """ | ||||
| # Replicate the parent's behavior to avoid calling a method | ||||
| # that is known to be unimplemented. | ||||
| raise NotImplementedError( | ||||
| "PyOpenSSL's Connection class does not support the makefile() interface", | ||||
| ) | ||||
|
|
||||
| def shutdown(self, how=None): | ||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we should have something for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unlike There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm mostly curious about the calling code using it rather than us reimplementing it. This class will get the implementation through inheritance. @julianz- I just remembered that the calling code actually invokes Line 1543 in 22bb430
I wonder if we could have interface that would be uniform across adapters and wouldn't have that ugly There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe then we assume all connection adapters (including SSLConnection) implement sock_shutdown? |
||||
| """Shutdown the SSL connection. | ||||
|
|
||||
| class SSLConnection(metaclass=SSLConnectionProxyMeta): | ||||
| r"""A thread-safe wrapper for an ``SSL.Connection``. | ||||
| :param how: Ignored. PyOpenSSL's | ||||
| :py:meth:`~OpenSSL.SSL.Connection.shutdown` | ||||
| method does not accept any arguments. | ||||
| Present here for interface compatibility with Python | ||||
| :py:meth:`~socket.socket.shutdown` that | ||||
| :py:class:`ssl.SSLSocket` wrapper exposes. | ||||
| :type how: object | ||||
| """ | ||||
| return super().shutdown() | ||||
|
|
||||
| :param tuple args: the arguments to create the wrapped \ | ||||
| :py:class:`SSL.Connection(*args) \ | ||||
| <pyopenssl:OpenSSL.SSL.Connection>` | ||||
| """ | ||||
| def sock_shutdown(self, how=None): | ||||
| """Shutdown the SSL connection. | ||||
|
|
||||
| def __init__(self, *args): | ||||
| """Initialize SSLConnection instance.""" | ||||
| self._ssl_conn = SSL.Connection(*args) | ||||
| self._lock = threading.RLock() | ||||
| This method is provided for interface compatibility and delegates | ||||
| directly to the standard shutdown() method. | ||||
| """ | ||||
| # We call self.shutdown(how) to use the method where we added | ||||
| # the compatibility logic (handling 'how=None' for the parent). | ||||
| return self.shutdown(how) | ||||
|
|
||||
|
|
||||
| class pyOpenSSLAdapter(Adapter): | ||||
|
|
||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm actually not sure if this constitutes a breaking change. SemVer talks about breaking changes in the context of API compatibility and the API seems to have remained intact. WDYT? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the API offered thread-safety and now suddenly we are saying these methods are not thread-safe that sounds like a breaking change to me, no? It's somewhat moot I guess if, in practice, no-one needs these methods to be thread-safe but just in case it's better to clear this is a potentially breaking change. If not a breaking change what category would be best? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Honestly, I'm torn. We could keep it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure. That makes sense. I'm fine to move to misc. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @julianz- I'm not opposed to keeping it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is Breaking Bad? haha. i'm torn also. i changed to |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| Removed thread-safety locking logic from | ||
| :py:class:`cheroot.ssl.pyopenssl.SSLConnection` as | ||
| this wrapper did not need to be thread-safe. | ||
|
|
||
| -- by :user:`julianz-` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this? Can we replicate the method signature exactly?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makefile() I added because the linter complained about makefile being abstract and needing an implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which linter? Do you have logs?