Improve SSL performance by avoiding SSLWantReadError exception and using much faster checks whenever possible

[tarasko]

SSLWantReadError is expensive.

python/cpython#123954

This PR tries to predict that there will be SSLWantReadError by checking incoming.pending and SSLObject.pending() first.
This check works in 99% of cases. For the rest 1% we still rely on SSLWantReadError

[andersfylling]

Can you expend on how you figured out it handles 99% of cases, and what are the last 1%?

[tarasko]

99% - 1% is figurative. I don't know the real numbers and obviously it depends on a particular use case.

Checking incoming.pending > 0 or SSLObject.pending() > 0 may not help if we've received only a part of SSL frame. I think it may happen if system TCP stack broke it up or some receiving buffer was too small. In such case incoming.pending > 0 or SSLObject.pending() > 0 will be True but when we call SSLObject.read there will be SSLWantRead exception anyway.

In my use case client and server exchange a lot of small messages and every message (every SSL frame) can fit into a single TCP packet. Incoming data can also fit into all receiving buffers. So it never happens that uvloop reads only a part of SSL frame, it is always a complete frame, and SSLObject.read is able to process all incoming data.

Checking incoming.pending > 0 or SSLObject.pending() is very cheap comparing to raising and catching SSLWantRead. I think it would be always good if we can prevent SSLWantRead.

[fantix]

This is a good one!

Though, it's trickier to verify correctness than just SSLWantReadError, because SSLObject.read() may also raise SSLWantWriteError during close_notify and re-negotiation. The two points to verify are:

1. If SSLObject.read() should be called regardlessly per _do_read().
2. If SSLObject.read() should be called regardlessly per loop within _do_read().

(1) is triggered by I/O read event or user calling transport.resume_reading(). In such scenarios, I don't see a case where the SSLObject has a pending state that requires read() to handle, if both buffers are empty.

(2) is where this optimization happens. It challenges the SSLObject state machine assumption, which is, to call read() until it returns 0, or raises an error. In other words, if a first read() call didn't return 0 or raise an error, can we safely skip the next read() call if both buffers are empty? Reading the underlying C code, I don't see any state left behind. And our use case here (sslproto.pyx) doesn't have other changes to the SSLObject state during _do_read(). Also, given that our sophisticated test cases are passing, I believe it's safe to apply this optimization.

[fantix]

Suggested change

	app_buffer_size, app_buffer)
	app_buffer_size - total_bytes_read, app_buffer)

[tarasko]

That's a good one. Fixed!

[tarasko]

Actually, probably doesn't make a difference because SSLObject.read won't read more than the length of app_buffer and it is already app_buffer_size - total_bytes_read. I fixed it anyway for consistency

[tarasko]

Yes, this whole SSLWantWriteError from SSLObject.read is tricky.

I'm not 100% sure but I have the following assumption:

1. Python ssl.MemoryBIO grows dynamically without fixed capacity, has no limit, write can only fail in case of system out-of-memory

2. SSL_read_ex returns SSL_WANT_WRITE_ERROR when memory BIO runs out of memory, not when we just wrote something. So given (1) SSL_WANT_WRITE_ERROR may only happen when the system runs out of memory.

3. SSL_read_ex does use its chance to write control stuff right after successful read before returning. link

[fantix]

Suggested change

	# self._incoming.pending > 0 SSLObject.pending() == 0,
	# self._incoming.pending > 0 and SSLObject.pending() == 0,

[fantix]

Ahh, good catch! This fixes a false EOF bug in previous code I think. Would be nice to have a test!