Add method to be able to hook in SNIMatchers that exists since java8 #247
Conversation
| TCN_ASSERT(ctx != 0); | ||
|
|
||
| if (matcher == NULL) { | ||
| SSL_CTX_set_tlsext_servername_callback(c->ctx, NULL); |
There was a problem hiding this comment.
Not a big deal, but maybe we should just always do this first? ... and then check if (matcher != NULL) for the rest? That way if method == null below, these will already have been properly cleared.
WDYT?
There was a problem hiding this comment.
@nmittler so you say we should always just set to NULL first and then maybe set to something not null if needed later in the method ?
There was a problem hiding this comment.
@normanmaurer yeah that was the thought ... WDYT?
There was a problem hiding this comment.
not sure this is really needed. I think I would prefer to keep it like it is...
@Scottmitch WDYT ?
There was a problem hiding this comment.
I'm fine with setting the value only when necessary.
However shouldn't we always check if we need to free memory?
if (c->sni_hostname_matcher != NULL) {
(*e)->DeleteGlobalRef(e, c->sni_hostname_matcher);
c->sni_hostname_matcher = NULL;
}
| if (c->sni_hostname_matcher != NULL) { | ||
| (*e)->DeleteLocalRef(e, c->sni_hostname_matcher); | ||
| } | ||
| c->sni_hostname_matcher = (*e)->NewGlobalRef(e, matcher); |
There was a problem hiding this comment.
Don't know if it's applicable but maybe NewWeakGlobalRef() instead of NewGlobalRef?
c->sni_hostname_matcher = (*e)->NewWeakGlobalRef(e, matcher);jobject matcher = (*e)->NewLocalRef(e, (jweak)c->sni_hostname_matcher);
if (!(*e)->IsSameObject(e, matcher, NULL)) {
// Do something with the SNIMatcher
} else {
// It appears the SNIMatcher got GC'd in Java land
}
(*e)->DeleteLocalRef(e, matcher);There was a problem hiding this comment.
I think a a GlobalRef is ok
| } | ||
| // Delete the reference to the previous specified matcher if needed. | ||
| if (c->sni_hostname_matcher != NULL) { | ||
| (*e)->DeleteLocalRef(e, c->sni_hostname_matcher); |
There was a problem hiding this comment.
I think this is a bug, shouldn't it be DeleteGlobalRef?
There was a problem hiding this comment.
You are right... doh!
normanmaurer
force-pushed
the
sni_matcher
branch
from
50d1c3a
to
88fb46b
Compare
|
@Scottmitch PTAL |
|
@trustin PTAL as well |
| TCN_ASSERT(ctx != 0); | ||
|
|
||
| if (matcher == NULL) { | ||
| SSL_CTX_set_tlsext_servername_callback(c->ctx, NULL); |
There was a problem hiding this comment.
I'm fine with setting the value only when necessary.
However shouldn't we always check if we need to free memory?
if (c->sni_hostname_matcher != NULL) {
(*e)->DeleteGlobalRef(e, c->sni_hostname_matcher);
c->sni_hostname_matcher = NULL;
}
Motivation: Java8 supports the concept of SNIMatcher, we should allow to hook these into tcnative. Modifications: Add new method and interface to allow using SNIMatcher in netty when using java8+ Result: More inline with features provided by java8+
normanmaurer
force-pushed
the
sni_matcher
branch
from
88fb46b
to
1959135
Compare
|
Cherry-picked into master (563307a) |
|
See PR #262 for followup |
Motivation:
Java8 supports the concept of SNIMatcher, we should allow to hook these into tcnative.
Modifications:
Add new method and interface to allow using SNIMatcher in netty when using java8+
Result:
More inline with features provided by java8+