You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2020/06/28 01:07:55 UTC
[GitHub] [lucene-solr] noblepaul opened a new pull request #1624: use MethodHandles in AnnotatedAPI
noblepaul opened a new pull request #1624:
URL: https://github.com/apache/lucene-solr/pull/1624
@uschindler Please review
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] uschindler commented on a change in pull request #1624: use MethodHandles in AnnotatedAPI
Posted by GitBox <gi...@apache.org>.
uschindler commented on a change in pull request #1624:
URL: https://github.com/apache/lucene-solr/pull/1624#discussion_r446632932
##########
File path: solr/core/src/java/org/apache/solr/api/AnnotatedApi.java
##########
@@ -113,7 +116,7 @@ public EndPoint getEndPoint() {
return Collections.singletonList(new AnnotatedApi(specProvider, endPoint, commands, null));
} else {
List<Api> apis = new ArrayList<>();
- for (Method m : klas.getDeclaredMethods()) {
+ for (Method m : klas.getMethods()) {
EndPoint endPoint = m.getAnnotation(EndPoint.class);
if (endPoint == null) continue;
if (!Modifier.isPublic(m.getModifiers())) {
Review comment:
The if statement is not needed, because the public checks are done by unreflect().
This code is from reflection times to catch error early. By using method handles everything is checked and linked early, so you can't get any surprises later. So the public checks is complete obsolete, all this is handled by unreflect.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #1624: use MethodHandles in AnnotatedAPI
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1624:
URL: https://github.com/apache/lucene-solr/pull/1624#discussion_r446625933
##########
File path: solr/core/src/java/org/apache/solr/api/AnnotatedApi.java
##########
@@ -225,7 +228,11 @@ public void call(SolrQueryRequest req, SolrQueryResponse rsp) {
if (Modifier.isPublic(method.getModifiers())) {
this.command = command;
this.obj = obj;
- this.method = method;
+ try {
+ this.method = MethodHandles.publicLookup().unreflect(method);
+ } catch (IllegalAccessException e) {
+ throw new RuntimeException("Unable to unlookup method");
Review comment:
Unrelated to to this PR (i.e. comment holds for previous version of the code), maybe `InterruptedException` should be caught separately and `Thread.currentThread().interrupt()` be called.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #1624: use MethodHandles in AnnotatedAPI
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1624:
URL: https://github.com/apache/lucene-solr/pull/1624#discussion_r446626404
##########
File path: solr/core/src/java/org/apache/solr/api/AnnotatedApi.java
##########
@@ -225,7 +228,11 @@ public void call(SolrQueryRequest req, SolrQueryResponse rsp) {
if (Modifier.isPublic(method.getModifiers())) {
this.command = command;
this.obj = obj;
- this.method = method;
+ try {
+ this.method = MethodHandles.publicLookup().unreflect(method);
+ } catch (IllegalAccessException e) {
+ throw new RuntimeException("Unable to unlookup method");
Review comment:
unlookup? (did you mean unreflect?)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] uschindler commented on a change in pull request #1624: use MethodHandles in AnnotatedAPI
Posted by GitBox <gi...@apache.org>.
uschindler commented on a change in pull request #1624:
URL: https://github.com/apache/lucene-solr/pull/1624#discussion_r446632522
##########
File path: solr/core/src/java/org/apache/solr/api/AnnotatedApi.java
##########
@@ -113,7 +116,7 @@ public EndPoint getEndPoint() {
return Collections.singletonList(new AnnotatedApi(specProvider, endPoint, commands, null));
} else {
List<Api> apis = new ArrayList<>();
- for (Method m : klas.getDeclaredMethods()) {
+ for (Method m : klas.getMethods()) {
EndPoint endPoint = m.getAnnotation(EndPoint.class);
if (endPoint == null) continue;
if (!Modifier.isPublic(m.getModifiers())) {
Review comment:
Yes. For both branches.
In 8.x we have to fix the class accessibility check (unrelated to the if statement).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] noblepaul commented on a change in pull request #1624: use MethodHandles in AnnotatedAPI
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1624:
URL: https://github.com/apache/lucene-solr/pull/1624#discussion_r447412157
##########
File path: solr/core/src/java/org/apache/solr/api/AnnotatedApi.java
##########
@@ -87,16 +88,18 @@ public EndPoint getEndPoint() {
public static List<Api> getApis(Object obj) {
return getApis(obj.getClass(), obj);
}
- public static List<Api> getApis(Class<? extends Object> klas , Object obj) {
- if (!Modifier.isPublic(klas.getModifiers())) {
- throw new RuntimeException(klas.getName() + " is not public");
+ public static List<Api> getApis(Class<? extends Object> theClass , Object obj) {
+ Class<?> klas = null;
+ try {
+ klas = MethodHandles.publicLookup().accessClass(theClass);
+ } catch (IllegalAccessException e) {
+ throw new RuntimeException(klas.getName() + " is not public", e);
Review comment:
Thanks, everyone. I wanted to get a quick review for the implementation. Basically, I was not too sure about the idioms we normally use for `MethodHandle` based reflection code. Moreover, We are more worried about the security manager complaining and wanted to do a PoC if that problem goes away
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] uschindler commented on a change in pull request #1624: use MethodHandles in AnnotatedAPI
Posted by GitBox <gi...@apache.org>.
uschindler commented on a change in pull request #1624:
URL: https://github.com/apache/lucene-solr/pull/1624#discussion_r446632680
##########
File path: solr/core/src/java/org/apache/solr/api/AnnotatedApi.java
##########
@@ -225,7 +228,11 @@ public void call(SolrQueryRequest req, SolrQueryResponse rsp) {
if (Modifier.isPublic(method.getModifiers())) {
this.command = command;
this.obj = obj;
- this.method = method;
+ try {
+ this.method = MethodHandles.publicLookup().unreflect(method);
+ } catch (IllegalAccessException e) {
+ throw new RuntimeException("Unable to unlookup method");
Review comment:
The exception text should not mention internal details. It should just say that the method cannot be called, as it's not accessible (non public, class not visible, different classloader,...)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #1624: use MethodHandles in AnnotatedAPI
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1624:
URL: https://github.com/apache/lucene-solr/pull/1624#discussion_r446631311
##########
File path: solr/core/src/java/org/apache/solr/api/AnnotatedApi.java
##########
@@ -113,7 +116,7 @@ public EndPoint getEndPoint() {
return Collections.singletonList(new AnnotatedApi(specProvider, endPoint, commands, null));
} else {
List<Api> apis = new ArrayList<>();
- for (Method m : klas.getDeclaredMethods()) {
+ for (Method m : klas.getMethods()) {
EndPoint endPoint = m.getAnnotation(EndPoint.class);
if (endPoint == null) continue;
if (!Modifier.isPublic(m.getModifiers())) {
Review comment:
So you agree @uschindler that the `if` can be removed in the `master` branch we're looking at, did I understand you correctly?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #1624: use MethodHandles in AnnotatedAPI
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1624:
URL: https://github.com/apache/lucene-solr/pull/1624#discussion_r446627210
##########
File path: solr/core/src/java/org/apache/solr/api/AnnotatedApi.java
##########
@@ -113,7 +116,7 @@ public EndPoint getEndPoint() {
return Collections.singletonList(new AnnotatedApi(specProvider, endPoint, commands, null));
} else {
List<Api> apis = new ArrayList<>();
- for (Method m : klas.getDeclaredMethods()) {
+ for (Method m : klas.getMethods()) {
EndPoint endPoint = m.getAnnotation(EndPoint.class);
if (endPoint == null) continue;
if (!Modifier.isPublic(m.getModifiers())) {
Review comment:
Is this still needed? Wouldn't `MethodHandles.publicLookup().accessClass(theClass)` only return `public` methods?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #1624: use MethodHandles in AnnotatedAPI
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1624:
URL: https://github.com/apache/lucene-solr/pull/1624#discussion_r446626784
##########
File path: solr/core/src/java/org/apache/solr/api/AnnotatedApi.java
##########
@@ -306,7 +313,7 @@ void invoke(SolrQueryRequest req, SolrQueryResponse rsp, CommandOperation cmd) {
} catch (InvocationTargetException ite) {
log.error("Error executing command ", ite);
throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, ite.getCause());
- } catch (Exception e) {
+ } catch (Throwable e) {
Review comment:
I think a comment referencing the `MethodHandle` Javadoc would be helpful here, as the Javadoc explains `Throwable` should be caught and the exception wrapped.
> But in Java source code, methods which perform method handle calls must either explicitly throw Throwable, or else must catch all throwables locally, rethrowing only those which are legal in the context, and wrapping ones which are illegal.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] uschindler commented on a change in pull request #1624: use MethodHandles in AnnotatedAPI
Posted by GitBox <gi...@apache.org>.
uschindler commented on a change in pull request #1624:
URL: https://github.com/apache/lucene-solr/pull/1624#discussion_r446890033
##########
File path: solr/core/src/java/org/apache/solr/api/AnnotatedApi.java
##########
@@ -306,7 +313,7 @@ void invoke(SolrQueryRequest req, SolrQueryResponse rsp, CommandOperation cmd) {
} catch (InvocationTargetException ite) {
Review comment:
This catch block can go away!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #1624: use MethodHandles in AnnotatedAPI
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1624:
URL: https://github.com/apache/lucene-solr/pull/1624#discussion_r446623850
##########
File path: solr/core/src/java/org/apache/solr/api/AnnotatedApi.java
##########
@@ -87,16 +88,18 @@ public EndPoint getEndPoint() {
public static List<Api> getApis(Object obj) {
return getApis(obj.getClass(), obj);
}
- public static List<Api> getApis(Class<? extends Object> klas , Object obj) {
- if (!Modifier.isPublic(klas.getModifiers())) {
- throw new RuntimeException(klas.getName() + " is not public");
+ public static List<Api> getApis(Class<? extends Object> theClass , Object obj) {
+ Class<?> klas = null;
+ try {
+ klas = MethodHandles.publicLookup().accessClass(theClass);
+ } catch (IllegalAccessException e) {
+ throw new RuntimeException(klas.getName() + " is not public", e);
Review comment:
If the exception is thrown, `klas` will be null and `klass.getName()` will throw NPE
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] uschindler commented on a change in pull request #1624: use MethodHandles in AnnotatedAPI
Posted by GitBox <gi...@apache.org>.
uschindler commented on a change in pull request #1624:
URL: https://github.com/apache/lucene-solr/pull/1624#discussion_r446632093
##########
File path: solr/core/src/java/org/apache/solr/api/AnnotatedApi.java
##########
@@ -87,16 +88,18 @@ public EndPoint getEndPoint() {
public static List<Api> getApis(Object obj) {
return getApis(obj.getClass(), obj);
}
- public static List<Api> getApis(Class<? extends Object> klas , Object obj) {
- if (!Modifier.isPublic(klas.getModifiers())) {
- throw new RuntimeException(klas.getName() + " is not public");
+ public static List<Api> getApis(Class<? extends Object> theClass , Object obj) {
+ Class<?> klas = null;
+ try {
+ klas = MethodHandles.publicLookup().accessClass(theClass);
+ } catch (IllegalAccessException e) {
+ throw new RuntimeException(klas.getName() + " is not public", e);
Review comment:
Good catch. The code here is a mess, as you declare the variable as null. You should declare it final before catch block and not assign null. Then compiler would have been complained.
The current code is a horrible antipattern. Always declare variables as final before try and assign once.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] uschindler commented on a change in pull request #1624: use MethodHandles in AnnotatedAPI
Posted by GitBox <gi...@apache.org>.
uschindler commented on a change in pull request #1624:
URL: https://github.com/apache/lucene-solr/pull/1624#discussion_r446631519
##########
File path: solr/core/src/java/org/apache/solr/api/AnnotatedApi.java
##########
@@ -306,7 +313,7 @@ void invoke(SolrQueryRequest req, SolrQueryResponse rsp, CommandOperation cmd) {
} catch (InvocationTargetException ite) {
log.error("Error executing command ", ite);
throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, ite.getCause());
- } catch (Exception e) {
+ } catch (Throwable e) {
Review comment:
There are two things to do:
- have the try-catch only around the invoke and nothing else, so we don't accidentally catch any unwanted exception in the surrounding code
- only checked exceptions should be wrapped, everything else rethrown. This keeps stack traces clean.
The reason for the declaration of "Throwable" comes from the fact that the method handle always throws the original Exception and it's now wrapped like for reflection. As checked exceptions is a compiler thing, normal code using method handles in bytecode or invokedynamic never needs to catch Throwable. It's only the stupid Java compiler that enforces us to catch everything, because it can't know if the handle might possibly throw a checked exception. When writing scripting languages you just ignore the Throwable when you produce bytecode. 🤓
Here we should catch all possible exceptions that are commonly thrown by plugins: SolrException, Runtime exception, Error and rethrown those. The last catch block would catch Throwable and log it separately as some fatal error.
The code above should check the possible exceptions a method may throw and forbid e.g. a plugin to throw Interrupted exception. So maybe get all throws clauses of method and have whitelist. In the catch clause then throw AssertionError if the "unknown" exception happens.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] noblepaul commented on a change in pull request #1624: use MethodHandles in AnnotatedAPI
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1624:
URL: https://github.com/apache/lucene-solr/pull/1624#discussion_r447736459
##########
File path: solr/core/src/java/org/apache/solr/api/AnnotatedApi.java
##########
@@ -87,16 +83,18 @@ public EndPoint getEndPoint() {
public static List<Api> getApis(Object obj) {
return getApis(obj.getClass(), obj);
}
- public static List<Api> getApis(Class<? extends Object> klas , Object obj) {
- if (!Modifier.isPublic(klas.getModifiers())) {
- throw new RuntimeException(klas.getName() + " is not public");
+ public static List<Api> getApis(Class<? extends Object> theClass , Object obj) {
+ Class<?> klas = null;
Review comment:
how is that possible?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #1624: use MethodHandles in AnnotatedAPI
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1624:
URL: https://github.com/apache/lucene-solr/pull/1624#discussion_r446623850
##########
File path: solr/core/src/java/org/apache/solr/api/AnnotatedApi.java
##########
@@ -87,16 +88,18 @@ public EndPoint getEndPoint() {
public static List<Api> getApis(Object obj) {
return getApis(obj.getClass(), obj);
}
- public static List<Api> getApis(Class<? extends Object> klas , Object obj) {
- if (!Modifier.isPublic(klas.getModifiers())) {
- throw new RuntimeException(klas.getName() + " is not public");
+ public static List<Api> getApis(Class<? extends Object> theClass , Object obj) {
+ Class<?> klas = null;
+ try {
+ klas = MethodHandles.publicLookup().accessClass(theClass);
+ } catch (IllegalAccessException e) {
+ throw new RuntimeException(klas.getName() + " is not public", e);
Review comment:
You obviously meant `theClass.getName()` here and not `klas.getName()`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] chatman commented on pull request #1624: use MethodHandles in AnnotatedAPI
Posted by GitBox <gi...@apache.org>.
chatman commented on pull request #1624:
URL: https://github.com/apache/lucene-solr/pull/1624#issuecomment-650691271
With this PR, the security exceptions are gone. +1 to merge.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] uschindler commented on a change in pull request #1624: use MethodHandles in AnnotatedAPI
Posted by GitBox <gi...@apache.org>.
uschindler commented on a change in pull request #1624:
URL: https://github.com/apache/lucene-solr/pull/1624#discussion_r446631519
##########
File path: solr/core/src/java/org/apache/solr/api/AnnotatedApi.java
##########
@@ -306,7 +313,7 @@ void invoke(SolrQueryRequest req, SolrQueryResponse rsp, CommandOperation cmd) {
} catch (InvocationTargetException ite) {
log.error("Error executing command ", ite);
throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, ite.getCause());
- } catch (Exception e) {
+ } catch (Throwable e) {
Review comment:
There are two things to do:
- have they try catch only around the invoke and nothing else, so we don't accidentally catch any unwanted exception in the surrounding code
- only checked exceptions should be wrapped, everything else rethrown. This keeps stack traces clean.
The reason for the declaration of "Throwable" comes from the fact that the method handle always throws the original Exception and it's now wrapped like for reflection. As checked exceptions is a compiler thing, normal code using method handles in bytecode or invokedynamic never needs to catch Throwable. It's only the stupid Java compiler that enforces us to catch everything, because it can't know if the handle might possibly throw a checked exception. When writing scripting languages you just ignore the Throwable when you produce bytecode. 🤓
Here we should catch all possible exceptions that are commonly thrown by plugins: SolrException, Runtime exception, Error and rethrown those. The last catch block would catch Throwable and log it separately as some fatal error.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1624: use MethodHandles in AnnotatedAPI
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1624:
URL: https://github.com/apache/lucene-solr/pull/1624#discussion_r446592665
##########
File path: solr/core/src/java/org/apache/solr/api/AnnotatedApi.java
##########
@@ -306,7 +313,7 @@ void invoke(SolrQueryRequest req, SolrQueryResponse rsp, CommandOperation cmd) {
} catch (InvocationTargetException ite) {
log.error("Error executing command ", ite);
throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, ite.getCause());
- } catch (Exception e) {
+ } catch (Throwable e) {
Review comment:
Catching Throwable is generally something to avoid. Why here do you think it makes sense?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] chatman commented on a change in pull request #1624: use MethodHandles in AnnotatedAPI
Posted by GitBox <gi...@apache.org>.
chatman commented on a change in pull request #1624:
URL: https://github.com/apache/lucene-solr/pull/1624#discussion_r446625191
##########
File path: solr/core/src/java/org/apache/solr/api/AnnotatedApi.java
##########
@@ -225,7 +228,11 @@ public void call(SolrQueryRequest req, SolrQueryResponse rsp) {
if (Modifier.isPublic(method.getModifiers())) {
this.command = command;
this.obj = obj;
- this.method = method;
+ try {
+ this.method = MethodHandles.publicLookup().unreflect(method);
+ } catch (IllegalAccessException e) {
+ throw new RuntimeException("Unable to unlookup method");
Review comment:
Lets throw SolrException here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] uschindler commented on a change in pull request #1624: use MethodHandles in AnnotatedAPI
Posted by GitBox <gi...@apache.org>.
uschindler commented on a change in pull request #1624:
URL: https://github.com/apache/lucene-solr/pull/1624#discussion_r447650963
##########
File path: solr/core/src/java/org/apache/solr/api/AnnotatedApi.java
##########
@@ -87,16 +83,18 @@ public EndPoint getEndPoint() {
public static List<Api> getApis(Object obj) {
return getApis(obj.getClass(), obj);
}
- public static List<Api> getApis(Class<? extends Object> klas , Object obj) {
- if (!Modifier.isPublic(klas.getModifiers())) {
- throw new RuntimeException(klas.getName() + " is not public");
+ public static List<Api> getApis(Class<? extends Object> theClass , Object obj) {
+ Class<?> klas = null;
Review comment:
this should be declared `final` and not nulled.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] uschindler commented on a change in pull request #1624: use MethodHandles in AnnotatedAPI
Posted by GitBox <gi...@apache.org>.
uschindler commented on a change in pull request #1624:
URL: https://github.com/apache/lucene-solr/pull/1624#discussion_r446630480
##########
File path: solr/core/src/java/org/apache/solr/api/AnnotatedApi.java
##########
@@ -113,7 +116,7 @@ public EndPoint getEndPoint() {
return Collections.singletonList(new AnnotatedApi(specProvider, endPoint, commands, null));
} else {
List<Api> apis = new ArrayList<>();
- for (Method m : klas.getDeclaredMethods()) {
+ for (Method m : klas.getMethods()) {
EndPoint endPoint = m.getAnnotation(EndPoint.class);
if (endPoint == null) continue;
if (!Modifier.isPublic(m.getModifiers())) {
Review comment:
Yes, we still need some way to check public methods. The accessClass() method above returns itsself, but the check for public is not needed if we use getMethods() instead of getDeclaredMethods().
The accessClass() check can't be backported to 8.x as it is new in Java 9. So we need to somehow do the check for accessibility manually.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] noblepaul commented on a change in pull request #1624: use MethodHandles in AnnotatedAPI
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1624:
URL: https://github.com/apache/lucene-solr/pull/1624#discussion_r446597787
##########
File path: solr/core/src/java/org/apache/solr/api/AnnotatedApi.java
##########
@@ -306,7 +313,7 @@ void invoke(SolrQueryRequest req, SolrQueryResponse rsp, CommandOperation cmd) {
} catch (InvocationTargetException ite) {
log.error("Error executing command ", ite);
throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, ite.getCause());
- } catch (Exception e) {
+ } catch (Throwable e) {
Review comment:
`methodhandle.invoke()` throws `Throwable`. What choice do we have?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] noblepaul commented on a change in pull request #1624: use MethodHandles in AnnotatedAPI
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1624:
URL: https://github.com/apache/lucene-solr/pull/1624#discussion_r447412157
##########
File path: solr/core/src/java/org/apache/solr/api/AnnotatedApi.java
##########
@@ -87,16 +88,18 @@ public EndPoint getEndPoint() {
public static List<Api> getApis(Object obj) {
return getApis(obj.getClass(), obj);
}
- public static List<Api> getApis(Class<? extends Object> klas , Object obj) {
- if (!Modifier.isPublic(klas.getModifiers())) {
- throw new RuntimeException(klas.getName() + " is not public");
+ public static List<Api> getApis(Class<? extends Object> theClass , Object obj) {
+ Class<?> klas = null;
+ try {
+ klas = MethodHandles.publicLookup().accessClass(theClass);
+ } catch (IllegalAccessException e) {
+ throw new RuntimeException(klas.getName() + " is not public", e);
Review comment:
Thanks, everyone. I wanted to get a quick review for the implementation. Basically, I was not too sure about the idioms we normally use for `MethodHandle` based reflection code.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] noblepaul merged pull request #1624: use MethodHandles in AnnotatedAPI
Posted by GitBox <gi...@apache.org>.
noblepaul merged pull request #1624:
URL: https://github.com/apache/lucene-solr/pull/1624
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org