I was using an Executor to limit the number of threads that consume this action to prevent the server from an SSL attack spinning on all the codes.
On Wed, Mar 2, 2022 at 6:43 PM Emmanuel Lécharny <[email protected]> wrote: > FTR, there is no point in using an executor to try to spread the load of > tasks across many threads: > > private static class DelegatedTask implements Runnable { > private final SSLEngineImpl engine; > > DelegatedTask(SSLEngineImpl engineInstance) { > this.engine = engineInstance; > } > > @Override > public void run() { > synchronized (engine) { <<------ This... > > Whatever we do, when a task is executed, it will block any other > DelegatedTask. > > > On 02/03/2022 15:33, Emmanuel Lécharny wrote: > > > > > > On 02/03/2022 13:57, Jonathan Valliere wrote: > >> > >> CONFIDENTIALITY NOTICE: The contents of this email message and any > >> attachments are intended solely for the addressee(s) and may contain > >> confidential and/or privileged information and may be legally > >> protected from disclosure. > >> > >> > >> On Wed, Mar 2, 2022 at 7:33 AM Emmanuel Lécharny <[email protected] > >> <mailto:[email protected]>> wrote: > >> > >> > >> > >> On 02/03/2022 12:36, Jonathan Valliere wrote: > >> > It’s already running in an Executor > >> > >> Can you clarify ? What is running in an executor? > >> > >> > >> The SSLEngine delegated task is run in a thread pool executor outside > >> of the filterchain. > >> > https://github.com/apache/mina/blob/4fb5d0ee65c079efab08d66bd4c6897f76f39c4f/mina-core/src/main/java/org/apache/mina/filter/ssl/SSLHandlerG0.java#L617 > >> < > https://github.com/apache/mina/blob/4fb5d0ee65c079efab08d66bd4c6897f76f39c4f/mina-core/src/main/java/org/apache/mina/filter/ssl/SSLHandlerG0.java#L617> > > >> > >> > >> > >> which is why it’s a runnable > >> > >> Well, it's not related. You can create Runnable instances and > execute > >> them outside of an executor. > >> > >> . The > >> > Runnable defers the actual execution logic to the execute task > >> function > >> > which should be the one that is looping to perform all queued > >> tasks from > >> > the SSL Engine. > >> > >> Yes, but my point is that we could spawn threads inside the execute > >> task > >> (one thread per task) instead of creating an executor inside the > >> schedule_task. > >> > >> > >> It uses a configured Executor instance which is generally the same one > >> for every SSL session. > >> > https://github.com/apache/mina/blob/4fb5d0ee65c079efab08d66bd4c6897f76f39c4f/mina-core/src/main/java/org/apache/mina/filter/ssl/SSLHandler.java#L84 > >> < > https://github.com/apache/mina/blob/4fb5d0ee65c079efab08d66bd4c6897f76f39c4f/mina-core/src/main/java/org/apache/mina/filter/ssl/SSLHandler.java#L84> > > >> > >> > https://github.com/apache/mina/blob/4fb5d0ee65c079efab08d66bd4c6897f76f39c4f/mina-core/src/main/java/org/apache/mina/filter/ssl/SSLFilter.java#L72 > >> < > https://github.com/apache/mina/blob/4fb5d0ee65c079efab08d66bd4c6897f76f39c4f/mina-core/src/main/java/org/apache/mina/filter/ssl/SSLFilter.java#L72> > > >> > > > > Ok, so we are on the same page here. > > > > So the goal here is to spawn a thread that will process the > > delegatedTask and let the IoProcessor deal with another session. > > > > That's pretty much needed considering the potentially costly task done > > in the delegated task. > > > > > > > >> > >> > >> My fear is that bu creating a new thread for each schedule_task > >> call is > >> that we may have many pending threads waiting for the thread > >> executing > >> execute_task to be completed, because of the synchronized nature > >> of the > >> execute_task method. All in all, it seems to create a bottleneck > >> that is > >> going to be problematic, when the tasks are supposed to be ran > >> concurrently. > >> > >> Am I missing something ? > >> > >> > >> If more than one scheduled task could happen (which I doubt it > >> honestly) yes they would block on each other because the execute_task > >> function is synchronized. But we want to block here because the > >> delegated tasks MUST be executed in order, > > > > No, this is not required: > > > > "Multiple delegated tasks can be run in parallel." > > ( > https://docs.oracle.com/en/java/javase/17/docs/api/java.base/javax/net/ssl/SSLEngine.html#getDelegatedTask()) > > > > > > > Although it's not clear if they meant "multiple delegated tasks for > > different sessions" or "multiple delegated tasks for one session", I > > think the former is correct. > > > > The SSLEngine will manage the context, and any wrap/unwrap call will be > > blocked until all the tasks are executed. > > > > So my guess is that it makes sense to use another executor to process > > the tasks, such as in my proposal. > > > > Now, going back to the initial question - ie why do we need an executor > > when we execute a synchronized method - do you agree that my > > understanding is correct: this allow the IoProcessor thread to be freed > > for another session ? > > > > > > if a second scheduled task is created, it's a > >> guaranteed that any subsequent delegated tasks are actually executed > >> and not lost in the time between a previous execution of a delegated > >> task and the exit of the synchronized block in execute_task. We don't > >> want any weird race conditions here. > > > > I'm not sure we could face race condition when executing tasks. They are > > supposed to be safe from a concurrency PoV. > > > > > >> > >> I wanted to ensure that the async captured exception elevates to the > >> Filter exception handler in a very standard and known way. > > > > Now this is interesting. OTOH as soon as we catch an exception for any > > task being executed, we will process it. It *may* happen we have more > > than one, if we have more than one task being executed concurrently. But > > in this case, the session would just be shut down by the first exception > > being handled, no matter which one it is. > > > > A simple way > >> to force the handling of the exception on the filter-chain is to cause > >> a throwing of a special WriteRequest which will flow down the filter > >> chain and cause the error to be thrown. You already see where I'm > >> > https://github.com/apache/mina/blob/4fb5d0ee65c079efab08d66bd4c6897f76f39c4f/mina-core/src/main/java/org/apache/mina/filter/ssl/SSLHandlerG0.java#L473 > >> < > https://github.com/apache/mina/blob/4fb5d0ee65c079efab08d66bd4c6897f76f39c4f/mina-core/src/main/java/org/apache/mina/filter/ssl/SSLHandlerG0.java#L473> > > >> writing the EncryptedWriteRequest from the async execute_task > >> function. Just create an EncryptedErrorRequest or some other object to > >> write and force the trigger. > > > > I'm not sure that is needed. Actually, the exception will pop up the > > chain, and the Handler will get it. What is important, and what is > > currently missing, is the alert that is not writen back to the remote > > peer (but that is the other thread's discussion). > > > > > > > > SSLEngine is a kind of nightmare... > > > > -- > *Emmanuel Lécharny - CTO* 205 Promenade des Anglais – 06200 NICE > T. +33 (0)4 89 97 36 50 > P. +33 (0)6 08 33 32 61 > [email protected] https://www.busit.com/ >
