-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 Mark,
On 11/22/18 18:24, Mark Thomas wrote: > > > On 22/11/2018 23:05, Christopher Schultz wrote: >> On 11/22/18 17:52, Mark Thomas wrote: >>> On 22/11/2018 22:32, Christopher Schultz wrote: >>>> On 11/22/18 16:43, Mark Thomas wrote: > > <snip/> > >>>>> syncs on encrypt() and decrypt() seem to have done the >>>>> trick. That was just a quick hack to confirm a suspicion - >>>>> it isn't the right long term fix. >>>>> >>>>> If we want this to be performant under load I'd lean >>>>> towards using a Queue for encryption ciphers and another >>>>> for decryption ciphers along the lines of the way >>>>> SessionIdGeneratorBase handles SecureRandom. >>>>> >>>>> We should probably handle SecureRandom the same way. >>>>> >>>>> I'll start working on a patch. >>>> >>>> Hmm... I was under the impression that the message-sending >>>> operations were single-threaded (and similar with the >>>> receiving operations). >>> >>> There are locks in other Interceptors which are consistent >>> with them being multi-threaded. >>> >>>> I've read that calling Cipher.init() is expensive, but since >>>> it's being done for every outgoing (and incoming) message, >>>> perhaps there is no reason to re-use Cipher objects. I'd be >>>> interested to see a micro-benchmark showing whether all that >>>> complexity (Cipher object pool) is really worth it. It >>>> probably isn't, given that the code without that complexity >>>> would be super clear and compact. >>> >>> Good point. I'll try that. It will depend on how expensive >>> creating the object is. >>> >>> Even with the pools the code is pretty clean but I take the >>> point that it would be (even) cleaner without. >>> >>> I have a patch ready to go but I'll do some work on the >>> benchmark first. >> >> I have a patch below. Passes all my unit-tests, but I don't have >> any multi-threaded ones written at this point. >> >> I'd appreciate a review before I commit. I'm going to change the >> cipher-pool-size to be configurable via an XML attribute, etc. > > The patch is hard to read. Can you upload it to people.a.o (or > maybe we should create a BZ issue for this). > > My benchmark shows similar results to yours. Pooling is certainly > worth while. > > I have a patch too. Available here: > http://people.apache.org/~markt/patches/20181122-encryption-intercepto r-concurrency-v1.patch > > > > It does a little more than just add pooling. > > It uses the same principle as elsewhere in Tomcat - that the pools > grows as needed and doesn't shrink so no need for additional > configuration options. > > I expect we'll pull bits from both patches but I'm going to call it > a day now and come back to this tomorrow. I took a slightly different approach, using a fixed-size ArrayBlockingQueue. I suppose we could use a LinkedBlockingQueue... it that fits-in better with how Tomcat does things in other scenarios, that's fine. My code defaulted to using a queue the size of the number of CPUs, since the encryption code is CPU-bound so a huge pool isn't really going to help even with thousands of request-processing threads. In terms of the Queue API... is there a big difference between using take/put versus poll/offer? I can't think of any case where we'd want to time-out waiting for an item (either checking-out or checking-in). For the stop() method... does it matter what "kind" of stop is being processed? We only call initCiphers() on certain kinds of "start" events, so we should probably tear everything down under the same conditions. I have a multi-threaded unit test that showed me that XButeBuffer is also not thread-safe :) - -chris -----BEGIN PGP SIGNATURE----- Comment: Using GnuPG with Thunderbird - https://www.enigmail.net/ iQIzBAEBCAAdFiEEMmKgYcQvxMe7tcJcHPApP6U8pFgFAlv3QywACgkQHPApP6U8 pFgBbg//fX1G6zLFMlsBs9UT6anKzz7vgNt/0z6ZEIJgFQ9zJa3AL+Xbp8FmwIQH lZy7aqn+uRPRhRYobsHA24Xyz98z6PzQYbOgch6TGzCyWPJ+h0IGNZVHnvTeOGfO xstiv/G5Y4GCRkdCvEz1TXnArZOoKp7H3Q9ZvOEqj/AShqycEtWYMx4a3Jt44JbC 1nc79P496Gpska3lYhBJSyhWs9IBVWpfKucCSEThEqcZbZrtDw9hpsCNK2gIYYup IxtMHZmIgwEtNM8luS6rsbD6Pad4fgbysCeiAfM1wfOmjOaaUU+6JCnv1zAEidGW TuofTCUvMFotVI/A3tAjxzXFVMi8kP329tFJBzzvmR2ka2D2SanGQIabJG+f3cWQ 7wJiV94cUGOhGpI3tYc6EmS98e9VxR24qv9wHV6gPu5dW5pDNQxjjDHALhnR4Uqq nIYk5L25iG+YYVQejWt30+vlnkLIPmic95nqx2ODNPN+f9g/21mQiAC2RGIMy415 QUKbt7BFv4P8ejSmcwFoy3rw02kJ2bfBU31rR741Tg6F3oeKed6GRc9N/W7XdaNy UPSoMel5QH5bLHJUXtbLRk05D/C1n55LQe500gvQQ1pUcaROPjbHg4et0y/eiBg8 5GMlN5WlFh4IAhZVrDCzibfqFeBIcX4pTS8rPqBJzhZcS4etgVs= =uRYu -----END PGP SIGNATURE----- --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
