Support to offload certificate selection to different thread.#430
Support to offload certificate selection to different thread.#430normanmaurer merged 2 commits intomasterfrom
Conversation
653bb46 to
faa0cf0
Compare
Motiviation: Sometimes selection of the certificate can take a long time as it may be stored on a remote system (for example). We should support to offload selection of the certificate to an other thread and be able to retry the SSL operation after it is done. This will also allow us to support the SSLEngine abstraction of SSLEngine.getDelegatingTask() and SSLEngineResult.HandshakeStatus.NEED_TASK. Modification: - Introduce SSLContest.setUseTasks(...) which allows to enable / disable the usage of tasks to offload certificate selection to an other thread. - Introduce SSL.getTask(...) which allows to retrieve the task that will do the certificate selection ( and also other tasks in the future ) Result: Be able to execute long running tasks for certificate selection ( and also others in the future) without blocking the I/O thread.
faa0cf0 to
9db7e04
Compare
| if (c->use_tasks != 0) { | ||
| // Lets create the CertificateCallbackTask and store it on the SSL object. We then later retrieve it via | ||
| // SSL.getTask(ssl) and run it. | ||
| jobject task = (*e)->NewObject(e, certificateCallbackTask_class, certificateCallbackTask_init, P2J(ssl), types, issuers, c->certificate_callback); |
There was a problem hiding this comment.
I think you need to check for an exception after this line.
There was a problem hiding this comment.
the constructor does not throw so I guess the only exception that could happen is a OOME.
There was a problem hiding this comment.
Can you put that as a comment in CertificateCallbackTask's ctor, so it's clear it can't throw in the future too?
| try { | ||
| callback.handle(ssl, keyTypeBytes, asn1DerEncodedPrincipals); | ||
| return 1; | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
It would be nice if this was logged at least.
There was a problem hiding this comment.
In netty we do before rethrow... we dont have any dependencies on a logging framework here. We could use java.util.logger ....
There was a problem hiding this comment.
That's what I was implying. Else, you could just rethrow it as RuntimeException.
There was a problem hiding this comment.
@carl-mastrangelo the problem is that rethrowing here is really "useless" as we will process it as part of an openssl c callback which needs to return 0 for an error to abort the handshake.
There was a problem hiding this comment.
@carl-mastrangelo also I think we don't want to log as we rethrow it on the Netty level correctly after the SSL.doHandshake(...) method returns and so fail the handshake with an SSLHandshakeException.
There was a problem hiding this comment.
so to make it short I think it is all fine as it is... Maybe I should add some comments tho ?
carl-mastrangelo
left a comment
There was a problem hiding this comment.
No other comments, generally LGTM
| if (c->use_tasks != 0) { | ||
| // Lets create the CertificateCallbackTask and store it on the SSL object. We then later retrieve it via | ||
| // SSL.getTask(ssl) and run it. | ||
| jobject task = (*e)->NewObject(e, certificateCallbackTask_class, certificateCallbackTask_init, P2J(ssl), types, issuers, c->certificate_callback); |
There was a problem hiding this comment.
Can you put that as a comment in CertificateCallbackTask's ctor, so it's clear it can't throw in the future too?
| // SSL.getTask(ssl) and run it. | ||
| jobject task = (*e)->NewObject(e, certificateCallbackTask_class, certificateCallbackTask_init, P2J(ssl), types, issuers, c->certificate_callback); | ||
| sslTask = (tcn_ssl_task_t*) OPENSSL_malloc(sizeof(tcn_ssl_task_t)); | ||
| sslTask->task = (*e)->NewGlobalRef(e, task); |
There was a problem hiding this comment.
sorry to be nitpicky, but this can fail too. Normally in Java I wouldn't care so much, but in C the risk is greater and it's much harder to debug.
| try { | ||
| callback.handle(ssl, keyTypeBytes, asn1DerEncodedPrincipals); | ||
| return 1; | ||
| } catch (Exception e) { |
|
@carl-mastrangelo addressed your comments |
Motiviation:
Sometimes selection of the certificate can take a long time as it may be stored on a remote system (for example). We should support to offload selection of the certificate to an other thread and be able to retry the SSL operation after it is done.
This will also allow us to support the SSLEngine abstraction of SSLEngine.getDelegatingTask() and SSLEngineResult.HandshakeStatus.NEED_TASK.
Modification:
Result:
Be able to execute long running tasks for certificate selection ( and also others in the future) without blocking the I/O thread.