-
-
Notifications
You must be signed in to change notification settings - Fork 9
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
Replace DefaultExecuteResultHandler with AtomicExecuteResultHandler #110
Conversation
9f45b64
to
cc9d4be
Compare
This includes the following noteworth changes: * vorburger/ch.vorburger.exec#110 * vorburger/ch.vorburger.exec#112 * vorburger/ch.vorburger.exec#115
This includes the following noteworth changes: * vorburger/ch.vorburger.exec#110 * vorburger/ch.vorburger.exec#112 * vorburger/ch.vorburger.exec#115
This includes the following noteworth changes: * vorburger/ch.vorburger.exec#110 * vorburger/ch.vorburger.exec#112 * vorburger/ch.vorburger.exec#115
This includes the following noteworth changes: * vorburger/ch.vorburger.exec#110 * vorburger/ch.vorburger.exec#112 * vorburger/ch.vorburger.exec#115
final long until = System.currentTimeMillis() + timeoutInMS; | ||
while (holder.get() == null && System.currentTimeMillis() < until) { | ||
Thread.sleep(SLEEP_TIME_MS); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a pretty minor nit, but for elapsed time it's safer to use System.nanoTime since currentTimeMillis is not monotone, and can flop around based on NTP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch! Thanks a lot for this feedback. This seemed easy enough to just quickly fix - I have raised #118 for it.
|
||
@Override | ||
public void onProcessComplete(int exitValue) { | ||
Holder witness = holder.compareAndExchange(null, new Holder(exitValue)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is slightly inconvenient for us–we're still on JDK8 (likely for the next year) so using AtomicReference#compareAndExchange (introduced in JDK9) means that we can't use ch.vorburger.exec anymore w/o forking it. We've already forked mariadb4j for JDK8 support so it's not hugely inconvenient, but I figured it's worth mentioning.
As an aside, do you prefer to use AtomicReference over VarHandle because it has a simpler API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* | ||
* @see <a href="https://github.com/vorburger/ch.vorburger.exec/issues/108">Issue #108</a> | ||
*/ | ||
public class AtomicExecuteResultHandler implements ExecuteResultHandler { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps this is a naive question–this seems a bit like a CompletableFuture. could we simplify this by using a CompletableFuture directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I vaguely see what you mean, but not entirely. That would (just) be "nicer design" but not per-se "more (concurrency) safe", is that what you mean? I am open to a PR Contrib for it, but personally not motivated enough to spend more time to change this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think the main improvement would be a more precise waitFor
method. I'm not sure how much that matters, but it could speed things up slightly. I'll take a stab at it if I get some free time
see #108
@mosesn (from #103) and @cardamon (from #96) and @duttonw FYI and hope you agree this makes sense?
Do raise follow-up PRs if you can see how to make it even better!