Hi,

I just saw that the changeset a79003f25a41 was merged and it adds support for memory BIO to the Python ssl module, great! It's a nice addition, especially because it becomes possible to implement SSL for IOCP event loop in asyncio (#22560).

I read the changeset and I have a few comments. Sorry, I didn't have time before to review the patch. I prefer to open a new issue. I'm interested to work on patches, but I would prefer a first feedback before working on a patch.


+.. method:: SSLSocket.read(len=0, buffer=None)

Hum, I fear that some people will call read() with two parameters by mistake. I would prefer a keyword-only parameter: put a "$" in the call to PyArg_ParseTuple() in PySSL_SSLread().

The "read()" method is quite generic, many Python functions expect a "file-like" object with a read() method which only take one indexed parameter.

An alternative would be to implemented readinto() and drop the buffer parameter from read().

Same remark for SSLObject.read() and _ssl._SSLSocket.read().


+.. attribute:: SSLSocket.server_hostname
+
+   A ``bytes`` instance (...)
+
+   .. versionadded:: 3.5

Oh, I would prefer to have a Unicode string here, but I see that the attribute is *not* new, it's already present in Python 3.4.

The versionadded field is wrong. It looks like the attribute was introduced in Python 3.2:
https://docs.python.org/dev/whatsnew/3.2.html#ssl

Maybe the change is that the attribute now also exists in the _ssl module? But the _ssl module is not documented.


+.. method:: MemoryBIO.write_eof()
+
+   Write an EOF marker to the memory BIO. After this method has been called, it
+   is illegal to call :meth:`~MemoryBIO.write`.

What does it mean, "illegal"? An exception is raised? Maybe it would be more explicit to say that an exception is raised.


+- All IO on an :class:`SSLObject` is non-blocking. This means that for example
+  :meth:`~SSLSocket.read` will raise an :exc:`SSLWantReadError` if it needs
+  more data than the incoming BIO has available.

It would be nice to repeat this information in the documentation of the SSLObject.read() method.


+.. method:: SSLContext.wrap_bio(incoming, outgoing, server_side=False, \

Why not puting the documentation of this method where the SSLContext is documented?


+Some notes related to the use of :class:`SSLObject`:

I suggest to move these notes just after the documentation of the SSLObject class.


+The following methods are available from :class:`SSLSocket`:

methods and *attributes*.


+class SSLObject:
+    """This class implements an interface on top of a low-level SSL object as
+    implemented by OpenSSL. This object captures the state of an SSL connection
+    but does not provide any network IO itself. IO needs to be performed
+    through separate "BIO" objects which are OpenSSL's IO abstraction layer.
+
+    This class does not have a public constructor. Instances are returned by
+    ``SSLContext.wrap_bio``. This class is typically used by framework authors
+    that want to implement asynchronous IO for SSL through memory buffers.
+
+    When compared to ``SSLSocket``, this object lacks the following features:
+
+     * Any form of network IO incluging methods such as ``recv`` and ``send``.
+     * The ``do_handshake_on_connect`` and ``suppress_ragged_eofs`` machinery.
+    """

This documentation is useful. It should be copied in the documentation of the SSLObject class.


+.. class:: MemoryBIO
+
+   A memory buffer that can be used to pass data between Python and an SSL
+   protocol instance.
+
+.. attribute:: MemoryBIO.pending
+
+   Return the number of bytes currently in the memory buffer.

I prefer when class methods and attributes are part of the "class" block in the documentation, the documentation is rendered differently and it's more readable IMO.

Compare:
   https://docs.python.org/dev/library/ssl.html#ssl.MemoryBIO
with:
   https://docs.python.org/dev/library/io.html#io.IOBase

But the choice how to document methods and attributes was not made in the memory BIO changeset, it is older. I suggest to "upgrade" to style to use the same style than the io module (put everything in the ".. class:" block).

What do you think?