Re: [Twisted-Python] log.callWithLogger not used - slows down reactor?

[Barry Scott]

On Wednesday, 3 June 2020 08:07:37 BST Glyph wrote:
> > On Jun 2, 2020, at 4:54 AM, Barry Scott 
> > wrote:
> > 
> > I'm hunting down performance issue in our code and spotted this in
> > passing. As far I can tell nothing seems to need callWithLogger.
> > 
> > I ran our 6k+ tests with this patch applied and everything worked.
> > 
> > Does anything in the twisted world need it?
> > 
> > Barry
> > 
> > diff --git a/src/twisted/internet/pollreactor.py b/src/twisted/internet/
> > pollreactor.py
> > index 6db1660b9..6901e5c95 100644
> > --- a/src/twisted/internet/pollreactor.py
> > +++ b/src/twisted/internet/pollreactor.py
> > @@ -165,7 +165,7 @@ class PollReactor(posixbase.PosixReactorBase,
> > 
> > posixbase._PollLikeMixin):
> > # Handles the infrequent case where one selectable's
> > # handler disconnects another.
> > continue
> > 
> > -log.callWithLogger(selectable, _drdw, selectable, fd, event)
> > +_drdw(selectable, fd, event)
> > 
> > doIteration = doPoll
> 
> I think that we may have eliminated all the dependency on it.  Do your logs
> look any different with this change applied?  If you can demonstrate its
> impact on speed.twistedmatrix.com  maybe
> we can go ahead and do this.  The logger used to be far more dependent on
> this, but the "system" portion of the log message is now based more on who
> is doing the logging than what socket provoked the event (which is what
> this is tracking, effectively).

Curious, if its dead code why do you need a speed test? For your comment
it seems you planned to stop using this.

What is it I would need to do for a speed test?

Barry


> 
> -glyph




___
Twisted-Python mailing list
Twisted-Python@twistedmatrix.com
https://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python

Re: [Twisted-Python] log.callWithLogger not used - slows down reactor?

[Glyph]

> On Jun 2, 2020, at 4:54 AM, Barry Scott  wrote:
> 
> I'm hunting down performance issue in our code and spotted this in
> passing. As far I can tell nothing seems to need callWithLogger.
> 
> I ran our 6k+ tests with this patch applied and everything worked.
> 
> Does anything in the twisted world need it?
> 
> Barry
> 
> diff --git a/src/twisted/internet/pollreactor.py b/src/twisted/internet/
> pollreactor.py
> index 6db1660b9..6901e5c95 100644
> --- a/src/twisted/internet/pollreactor.py
> +++ b/src/twisted/internet/pollreactor.py
> @@ -165,7 +165,7 @@ class PollReactor(posixbase.PosixReactorBase, 
> posixbase._PollLikeMixin):
> # Handles the infrequent case where one selectable's
> # handler disconnects another.
> continue
> -log.callWithLogger(selectable, _drdw, selectable, fd, event)
> +_drdw(selectable, fd, event)
> 
> doIteration = doPoll

I think that we may have eliminated all the dependency on it.  Do your logs 
look any different with this change applied?  If you can demonstrate its impact 
on speed.twistedmatrix.com  maybe we can go 
ahead and do this.  The logger used to be far more dependent on this, but the 
"system" portion of the log message is now based more on who is doing the 
logging than what socket provoked the event (which is what this is tracking, 
effectively).

-glyph

___
Twisted-Python mailing list
Twisted-Python@twistedmatrix.com
https://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python

Re: [Twisted-Python] [RFC] Drop support for Python 3.5 sometime after May 2021?

[Glyph]

> On May 28, 2020, at 3:13 PM, Tom Most  wrote:
> 
> On Fri, May 22, 2020, at 10:54 PM, Glyph wrote:
>> So, pidfd's cool, we should totally use it. Also we should use posix_spawn 
>> and maybe some other stuff too. But I wonder if there's any heuristic we 
>> could use to speed up our current strategy, like ordering the to-reap list 
>> by putting things with no open FDs at the front of it? And optimistically 
>> assuming that once we've found something to reap, maybe we can stop? And 
>> maybe it should run in a cooperator, rather than just blocking the reactor 
>> indefinitely?
> 
> Those all sound like reasonable optimizations with little downside to me.

Great!  Anybody feel like filing some tickets? :)

>> Honestly it had not occurred to me that people were managing 20k+ python 
>> interpreters at a time with spawnProcess (although, yikes, amazing, you've 
>> gotta talk more about this application, and what kind of hardware you're on!)
> 
> Nonono, that was a synthetic stress test. It did spawn Python processes, but 
> they immediately called `exec sleep` to conserve memory.
> 
> The result of the test was that the service would run out of memory _well_ 
> before inefficiencies in Twisted's reaping scheme became a problem.


Aah, that's less exciting ;).

-g
___
Twisted-Python mailing list
Twisted-Python@twistedmatrix.com
https://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python

Re: [Twisted-Python] Deprecate and remove t.internet.ssl.DistinguishedName ?

[Glyph]

> On May 29, 2020, at 11:16 PM, Wim Lewis  wrote:
> 
> I'm looking at a fix for bug 
> (Cannot load a PEM certificate with Unicode in subject). The underlying
> problem is that the DistinguishedName class can't handle non-ascii AVAs.
> The fix I've made simply avoids creating DistinguishedName instances when
> it isn't necessary, but that leaves the question of what to do with the
> class. I think that the best thing to do is to deprecate the class
> entirely and replace it with simpler API. 

Thanks for checking in about this!

> Reasons I think that the DN class is broken:
>  - The values in a certificate are conceptually text-strings, not
>byte strings; they may be in ASCII, UTF8, UTF16, or several
>other encodings. However
>- DN represents these textual values as `bytes` instead of `str`
>- DN can't handle non-ASCII-representable values at all, even if
>  the user never tries to access that value
>  - It can only handle a subset of the attribute-assertions found in
>a PKIX DN; there's no escape hatch for others (e.g. OID keys or
>whatever)
>  - It can't represent the full structure of a DN (specific ordering,
>multiple-value RDNs, AVAs whos values aren't textual, etc.) ---
>these are not common in the PKIX world but they are valid

Ugh.  I wrote this class where I knew literally nothing about TLS, and really 
just wanted it to be the thing it effectively is to the user (i.e. "connect to 
a hostname") rather than the thing it actually is (a tarpit of unnecessarily 
complex asn.1 nonsense).

Additionally, this was when pyOpenSSL was unmaintained and buggy as hell and 
X509Name would segfault when you so much as sneezed at it right.

> What I propose as an alternative:
>  - Replace APIs that take `DistinguishedName` classes with ones that take
>`Union[OpenSSL.crypto.X509Name, dict]` where the `dict` format is parsed
>with the same convenience semantics as DistinguishedName, except that
>values are `str`
>  - Replace APIs that return `DistinguishedName` with ones that return
>OpenSSL.crypto.X509Name, which is already fairly convenient to use
>(e.g. it has attributes for retrieving/setting commonName
>and so on without dealing with the full complexity of X.500 names)
>  - Deprecate `DistinguishedName` and the APIs that use it for eventual removal
>  - Expose a convenience function for the dict -> X509Name transform
> 
> Any objections? Thoughts on how I should go about doing this? Should I
> do it as part of this Trac ticket or split it out?
> 
> The only downside I can think of is that this exposes the
> OpenSSL.crypto.X509Name type as part of Twisted's API. I don't think
> this is a huge reduction in flexibility --- Twisted's API already somewhat
> assumes that TLS is implemented using OpenSSL, and only users whose needs
> are *already* not well met by DistinguishedName will care if that `Union`
> type changes in the future.

We've been slowly trying to paper over the aspects of the API that expose 
OpenSSL details and provide a more complete abstraction for years.  We are not 
there yet, as you observe!  We do still somewhat assume TLS uses OpenSSL.  But 
I really want to get away from that; I want to be able to use SChannel on 
Windows and Network.framework on macOS and provide a nice abstraction that 
floats above that.  This means - particularly in the latter case - not just 
eliminating the OpenSSL dependency, but actually adding new APIs that allow 
some pluggability on the reactor itself to allow for combining higher-level 
protocols together in the reactor itself.  (Maybe even HTTP, given the way that 
NSURLSession seems to want to provide HTTP3 as a platform service directly, 
skipping sockets and indeed TCP and TLS entirely...)

In a nearer-term, more practical way that we might leverage this soon, is a 
"lite" TLS provider that uses the stdlib's SSLSocket rather than all of 
pyOpenSSL; this would substantially reduce the dependency weight of Twisted on 
embedded platforms (for example, you can get the built-in SSL module in a 
context like https://omz-software.com/pythonista/ 
 but there's no way to get cryptography & 
pyOpenSSL at all).

One success story on this front is optionsForClientTLS().  Hopefully one day in 
the not too distant future we can get an optionsForServerTLS and slowly hide 
CertificateOptions underneath it.

So I would see this refactoring as an opportunity to move further away from 
pyOpenSSL dependency rather than further towards it.  I'm all for deprecating 
"DN", it's a terrible wrapper, but I think we could have a better wrapper.

That said; if abstracting this stuff away is really challenging, and you 
already have a fix close to ready to go, we can always evolve the API again 
when we do this for real, and have an actual second backend to prove the 
interface against.

-glyph


___