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/
