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

API improvements around Executor/Executable #1610

Merged
merged 20 commits into from Mar 25, 2015

Conversation

@jglick
Copy link
Member

jglick commented Mar 19, 2015

See jenkinsci/pipeline-plugin#95 for main use cases.

  • JENKINS-26090: Executables.getExecutor
  • JENKINS-26092: executors.jelly interface underspecified
  • JENKINS-25938: lock an Executor without creating a Thread
  • JENKINS-22941: way to mark an Executable that should not block isReadyToRestart
  • JENKINS-26900: hide flyweight master executor when ≥1 heavyweight executors running as subtasks

@reviewbybees

@jglick jglick changed the title [WiP] API improvements around Executor/Executable API improvements around Executor/Executable Mar 24, 2015
@jtnord

This comment has been minimized.

Copy link
Member

jtnord commented Mar 24, 2015

is throwing exceptions to indicate normal behavior the only way?
If it has come to this by keeping Jenkins API compatible is it not time to stand back and think about doing something different rather than bending things so that new and previously unthought of constructs can be shoe horned into the core?

@jglick

This comment has been minimized.

Copy link
Member Author

jglick commented Mar 24, 2015

is throwing exceptions to indicate normal behavior the only way?

Admittedly not ideal but seemed the simplest way to deal with an existing interface that defined a synchronous void run() method. In fact if you look at the new implementations, they start off running synchronously in the executor thread, and then if all goes well they switch to asynchronous mode for the remainder of the work. The executables can still take advantage of existing infrastructure surrounding Executor, they just need to avoid consuming a native thread, and make some minor customizations to other behaviors.

@jglick jglick mentioned this pull request Mar 24, 2015
11 of 11 tasks complete
stephenc added a commit to stephenc/jenkins that referenced this pull request Mar 25, 2015
stephenc added a commit to stephenc/jenkins that referenced this pull request Mar 25, 2015
@@ -88,6 +88,7 @@ public void execute(@Nonnull Runnable task, ResourceActivity activity ) throws I
try {
task.run();
} finally {
// TODO if AsynchronousExecution, do that later

This comment has been minimized.

Copy link
@stephenc

stephenc Mar 25, 2015

Member

You may need to revisit the locking after #1596

@@ -39,8 +39,9 @@ THE SOFTWARE.
<j:if test="${!c.acceptingTasks}"> <st:nbsp/> (${%suspended})</j:if>
</d:tag>
<d:tag name="executor">
<j:set var="ae" value="${e.asynchronousExecution}"/>

This comment has been minimized.

Copy link
@stephenc

stephenc Mar 25, 2015

Member

revisit against race conditions after #1596 is merged

This comment has been minimized.

Copy link
@jglick

jglick Mar 25, 2015

Author Member

I doubt that should have any relationship. If asynchronousExecution is set, the executor has already started, so any queue changes are long done. This is only displaying a snapshot of the executor state anyway.

This comment has been minimized.

Copy link
@stephenc

stephenc Mar 25, 2015

Member

And that was where I caught my NPEs... (the issue being that isIdle() can have changed after you have captured ae... really need an atomic state snapshot object that executor returns in order to display its state consistently

This comment has been minimized.

Copy link
@jglick

jglick Mar 25, 2015

Author Member

If we capture ae and then it goes idle, there is no harm done that I can see. But yes it would be nice to incorporate into DisplayExecutor.

This comment has been minimized.

Copy link
@Peggs

Peggs Mar 25, 2015

How do I stop getting these emails?

Sent from my iPhone

On 25 Mar 2015, at 15:25, Jesse Glick notifications@github.com wrote:

In core/src/main/resources/lib/hudson/executors.jelly:

@@ -39,8 +39,9 @@ THE SOFTWARE.
<j:if test="${!c.acceptingTasks}"> st:nbsp/ (${%suspended})/j:if
/d:tag
<d:tag name="executor">

  •  <j:set var="ae" value="${e.asynchronousExecution}"/>
    
    If we capture ae and then it goes idle, there is no harm done that I can see. But yes it would be nice to incorporate into DisplayExecutor.


Reply to this email directly or view it on GitHub.

This comment has been minimized.

Copy link
@orrc

orrc Mar 25, 2015

Member

@Peggs At the top of any page in this project, click the "Unwatch" button and choose "Not watching".

@stephenc

This comment has been minimized.

Copy link
Member

stephenc commented Mar 25, 2015

👍

@jtnord

This comment has been minimized.

Copy link
Member

jtnord commented Mar 25, 2015

I am ok (+1) on the code.
I am -.5 on the design of throwing exceptions to indicate asynchronous work - but can be persuaded to a +.5 if it is the only way it can be done in a backwards compatible way.

@jglick

This comment has been minimized.

Copy link
Member Author

jglick commented Mar 25, 2015

if it is the only way it can be done in a backwards compatible way

It is certainly not the only way; it felt like the most straightforward way I could come up with. Providing

interface Executable2 extends Executable {
    AsynchronousExecution run2();
}

would work, but then implementations still need to implement void run() to do something—assert false;? Making Executable2 not be related to Executable is not reasonable, because then there is no well-defined type for executables, which various parts of core code pass around.

Also for the known implementations, it is actually convenient to use the Executor thread which is already there to do the preparatory work (that sometimes actually fails with a regular exception or returns early), which would otherwise need to farmed out to an executor service or the like.

Ideally the original design would have not made Executor extends Thread to begin with, so Executor would merely be a logical slot, and you could decide to use whatever thread pool you liked to run your body. This would be a big and probably incompatible refactoring.

(Saving the allocation of a short-lived native thread would also be a benefit, but according to my tests on Ubuntu/Java 8 a trivial one: I compared a program that constructed 1_000_000 Threads and did not use them, to one which started them all but let them finish after a 50ms sleep, to one which started them and forced them to stay alive. The last of course got a process limit error around 30k threads. The first was instantaneous: Thread.<init> is basically free. I expected the second to be a lot slower, but in fact it only took a couple minutes including logging—negligible compared to other overheads involved in creating and running builds. jvisualvm confirmed that at any given time there were only a few dozen threads remaining in the “dead” state; jstack does not show them.)

@kohsuke

This comment has been minimized.

Copy link
Member

kohsuke commented Mar 25, 2015

One of the earlier prototypes of servlet 3.0 (and IIUC Jetty did it this way) was to throw an exception to indicate asynchronous processing, so I think there's some known precedent.

Another alternative is to do what servlet 3.0 is doing in the final version, that you set a flag before returning to indicate that you intend the processing to be asynchronous.

I think the current approach (of indicating the intent by throwing an exception) is fine.

* @return the executor (possibly a {@link OneOffExecutor}) whose {@link Executor#getCurrentExecutable} matches that, or null
* @since TODO
*/
public static @CheckForNull Executor getExecutor(Executable executable) {

This comment has been minimized.

Copy link
@kohsuke

kohsuke Mar 25, 2015

Member

Feels like this belongs more to the Executor class, like Executor.of(executable)

kohsuke and others added 2 commits Mar 25, 2015
IIUC, the expectation here is that Queue.Executable will instantiate AsynchronousException as a handle, start something asynchronously (say send a JMS message) with a callback, and the callback will tickle AsynchronousException.

So AsynchronousException might complete before it gets its executor set. This code change ensures that that case gets handled correctly.
Additional change suggestions on top of PR 1610
@kohsuke

This comment has been minimized.

Copy link
Member

kohsuke commented Mar 25, 2015

👍

@stephenc

This comment has been minimized.

Copy link
Member

stephenc commented Mar 25, 2015

I think this should be merged after #1596 given there are some race condition changes from @kohsuke

@jglick

This comment has been minimized.

Copy link
Member Author

jglick commented Mar 25, 2015

given there are some race condition changes from @kohsuke

63d3b26 does not have anything to do with Queue locking; only applies once the build is underway.

*/
public abstract class AsynchronousExecution extends RuntimeException {

private Executor executor;

This comment has been minimized.

Copy link
@stephenc

stephenc Mar 25, 2015

Member

@GuardedBy("this") if it really is guarded... ditto for result

// Make sure isQuietingDown is still true.
if (isQuietingDown) {
cleanUp();
System.exit(0);
}
} catch (InterruptedException e) {
} catch (Exception e) {
LOGGER.log(Level.WARNING, "Failed to shutdown Hudson",e);

This comment has been minimized.

Copy link
@stephenc

stephenc Mar 25, 2015

Member

When touching the line above I would have done an s/Hudson/Jenkins/

@@ -594,6 +653,32 @@ protected Object call(Object o, Method m, Object[] args) throws Throwable {
}

/**
* Finds the executor running a given process.

This comment has been minimized.

Copy link
@stephenc

stephenc Mar 25, 2015

Member

Finds the executor that was running a given process while this method was being called. There is the potential that the process may complete by the time this method returns and thus a non-null result could correspond to an Executor running a different process

This comment has been minimized.

Copy link
@jglick

jglick Mar 25, 2015

Author Member

I do not think so. As of 1.536, an Executor is thrown out after each use. Thus at worst it will return null for something that was running earlier. But this deserves clarification.

This comment has been minimized.

Copy link
@stephenc

stephenc Mar 25, 2015

Member

Well you can still have Executor executable = Executor.of(executable); assert executable.getCurrentExecutable() == executable give an Assertion error` as the executable can finish concurrent with returning from the method.

* @return the executor (possibly a {@link OneOffExecutor}) whose {@link Executor#getCurrentExecutable} matches that, or null
* @since TODO
*/
public static Executor of(Executable executable) {

This comment has been minimized.

Copy link
@stephenc

stephenc Mar 25, 2015

Member

@CheckForNull

@kohsuke

This comment has been minimized.

Copy link
Member

kohsuke commented Mar 25, 2015

👍

@recampbell

This comment has been minimized.

Copy link
Member

recampbell commented Mar 25, 2015

👍

1 similar comment
@stephenc

This comment has been minimized.

Copy link
Member

stephenc commented Mar 25, 2015

👍

jglick added a commit that referenced this pull request Mar 25, 2015
API improvements around Executor/Executable
@jglick jglick merged commit cfa4b10 into jenkinsci:master Mar 25, 2015
1 check passed
1 check passed
Jenkins This pull request looks good
Details
@jglick jglick deleted the jglick:Executable-JENKINS-25938 branch Mar 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.