*Cores
On Wed, Mar 2, 2022 at 8:31 PM Jonathan Valliere <john...@apache.org> wrote:
> 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 <elecha...@gmail.com>
> 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 <elecha...@gmail.com
>> >> <mailto:elecha...@gmail.com>> 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
>> emmanuel.lecha...@busit.com https://www.busit.com/
>>
>
