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]> 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 > > 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/SSLFilter.java#L72 > > 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, 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 wanted to ensure that the async captured exception elevates to the Filter exception handler in a very standard and known way. 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 writing the EncryptedWriteRequest from the async execute_task function. Just create an EncryptedErrorRequest or some other object to write and force the trigger. > > > > On Wed, Mar 2, 2022 at 3:51 AM Emmanuel Lécharny <[email protected] > > <mailto:[email protected]>> wrote: > > > > Hi, > > > > an alternative would be to modify the execute_task method to become: > > > > synchronized protected void execute_task(NextFilter next) { > > Runnable task = null; > > > > while ((task = mEngine.getDelegatedTask()) != null) { > > try { > > if (ENABLE_ASYNC_TASKS && (mExecutor != null)) { > > mExecutor.execute(task); > > } else { > > task.run(); > > } > > > > write_handshake(next); > > } catch (SSLException e) { > > > > So here, we push the tasks into an executor to be processed > > concurrently. > > > > On 02/03/2022 07:05, Emmanuel Lécharny wrote: > > > Hi Jonathan, > > > > > > I wonder if using an executor to process SSLEngine tasks is > > useful at > > > all, considering that the execute_task method is synchronized: > > > > > > protected void schedule_task(NextFilter next) { > > > if (ENABLE_ASYNC_TASKS) { > > > if (mExecutor == null) { > > > execute_task(next); > > > } else { > > > mExecutor.execute(new Runnable() { > > > @Override > > > public void run() { > > > SSLHandlerG0.this.execute_task(next); > > > } > > > }); > > > > > > and > > > > > > synchronized protected void execute_task(NextFilter next) { > > > ... > > > > > > > > > Also the execute_task will pull tasks from the SSLEngine - we may > > have > > > more than one, so it will loop on the tasks - and it's unlikely > > that we > > > will have more tasks to execute when exiting the execute_tasks > > method. > > > > > > The only benefit I can see is that the main thread will be freed > for > > > other tasks - like processing another IoSession - while tasks are > > being > > > processed by another thread. And may be this is what was > > intended, but I > > > wonder iff we should not simply spawn a dedicated thread for that > > purpose. > > > > > > IMO, if we want to introduce an executor in the equation, it > > should be > > > done in the execute_task method, not in the schedule_task, it > would > > > speed up the TLS processing, while also freeing the main thread > fast > > > enough - even a bit slower than having the task loop being > > processed by > > > a dedicated thread. > > > > > > wdyt? > > > > > > > -- > > *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] <mailto:[email protected]> > > https://www.busit.com/ <https://www.busit.com/> > > > > --------------------------------------------------------------------- > > To unsubscribe, e-mail: [email protected] > > <mailto:[email protected]> > > For additional commands, e-mail: [email protected] > > <mailto:[email protected]> > > > > -- > > 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. > > -- > *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/ >
