Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #1036 Configure Scheduler #4086

Merged
merged 3 commits into from
Sep 17, 2019
Merged

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Sep 12, 2019

Allows scheduler configuration of number of threads, name etc.

Signed-off-by: Greg Wilkins gregw@webtide.com

Allows scheduler configuration

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw gregw requested review from joakime and sbordet September 12, 2019 23:53
@gregw
Copy link
Contributor Author

gregw commented Sep 12, 2019

@sbordet Do you think we should take scheduler threads from the ThreadPool and include them in the thread budget?

@sbordet
Copy link
Contributor

sbordet commented Sep 16, 2019

@gregw no, I don't think we should budget scheduler threads. Typically there is only one.

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw gregw requested a review from sbordet September 17, 2019 02:41
@sbordet sbordet merged commit f85382a into jetty-9.4.x Sep 17, 2019
@sbordet sbordet deleted the jetty-9.4.x-1036-SchedulerThreads branch September 17, 2019 09:37
@rrorden
Copy link

rrorden commented Feb 5, 2020

The changes merged for this issue have uncovered what I believe is a bug in the code for the XmlConfiguration$JettyXmlConfiguration.construct(Class<?> klass, Object[] arguments, Map<String, Object> namedArgMap) method.

I ran into this while testing jetty 9.4.26 on the Perc Virtual Machine (PVM), a real-time commercial JVM that has been available since 1996 and which I help maintain. Perc is an alternative JVM like IBM J9 and others. We have customers using it with Jetty, Tomcat and other frameworks.

The modifications for issue #1036 and pull request #4086 include adding definitions to jetty.xml for constructing a ScheduledExecutorScheduler and adding parameter annotations to that constructor in order to match the specified in jetty.xml:
92e4d73

It also adds a new three-argument constructor to ScheduledExecutorScheduler:
public ScheduledExecutorScheduler(String name, boolean daemon, ClassLoader classLoader)

The problem is that XmlConfiguration$JettyXmlConfiguration.construct() calls Class.getConstructors() on ScheduledExecutorScheduler and then iterates through them until it finds one with the same number of parameters as the length of the passed in arguments array (3 in this case). Then it calls Constructor.getParameterAnnotations() in preparation to create a "swizzled" array of arguments that match up to the namedArgMap that was passed into the method. Unfortunately, the Java spec for Class.getConstructors() does not guarantee the order of the returned constructors. OpenJDK returns the newly parameterized (String, boolean, int) constructor first, but Perc happens to return the new (String, boolean, ClassLoader) constructor first and Constructor.getParameterAnnotations() returns an empty array. This causes construct() to debug-log "Invoking constructor, no parameter annotations" and invoke the wrong constructor.

If the intent is to be able to call a constructor with the xml-specified parameter types using annotations, then I believe the construct() method needs to keep searching for a constructor that a) has parameter annotations and b) those annotations match the namedArgMap elements. It certainly is wrong to select the constructor based solely on the number of parameters it takes.

@sbordet
Copy link
Contributor

sbordet commented Feb 13, 2020

@rrorden thanks for reporting this. Can you please open a new issue with your comment above?

@rrorden
Copy link

rrorden commented Feb 13, 2020

@rrorden thanks for reporting this. Can you please open a new issue with your comment above?

@gregw opened #4550 to address this issue. I was able to download and build a snapshot with his changes he made and now it works with the Perc JVM. Amazing response time! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants