A few small-ish things the I noticed (I should stop starting my answers with some sentence like this ...):
You're working on bare threads. That's usually considered a bad idea, because they don't clean up so easily. It's preferred to use
ExecutorService
and related classes. Interestingly you already do that for the prompt ...You're always starting the thread when
runFuturesConsumer
andrunKeyPressDaemon
are called. This is bad, because it's not documented anywhere and I'd expect the method to keep track of whether what you try to run is already running and not start another instance when it's not needed.You didn't post the code for
cancelServerFuture
but the naming suggests something unsettling. Cancelling usually means that there's no result, but your code implies there should be? Maybe?
I don't see much benefit in cancelling the server computation and then transferring something to the server (short-circuit does that). Or maybe I'm just missing something ...You're handling a user-cancel as a failure. I think that's misleading. A user cancellation is never a failure. Yet after the user cancelled the computation you display the message
"failure caused by: stopped before the completion"
. This is not really the expected behaviour. Instead I'd expect either no message or something to the effect of"Successfully cancelled execution"
.There's no code showing what exactly the Locks are that you're using, but judging by how they are used in the code, I'm not sure whether you actually need them in the first place and if you need them, whether you should lock that way ... From what I can make of the code, the only actually necessary lock should be the outputLock (which should also be used for the prompt)
The interruption process you use is something that's usually better known as
POISON_PILL
. At least that's the conventional name for it as I heard it.While we're on interruption: it's interesting that you compare
INTERRUPTION_VALUE
throughObject#equals
and the other cases through a reference equals (==
). I'm not quite sure why that is, but alas ...
While we're on interruption: it's interesting that you compare INTERRUPTION_VALUE
through Object#equals
and the other cases through a reference equals (==
). I'm not quite sure why that is, but alas ...
- You're overcomplicating your handling of the prompt. Instead of calling
handlePrompt
(and effectively nesting a switch block there) you could just "inline" the execution there. Additional note there: "incorrect" user input isn't handled there. Passing in "asd" just blows up the code with anInputMismatchException
:(You're overcomplicating your handling of the prompt. Instead of calling
handlePrompt
(and effectively nesting a switch block there) you could just "inline" the execution there. Additional note there: "incorrect" user input isn't handled there. Passing in "asd" just blows up the code with anInputMismatchException
:(
Well this should cover most of it, I strongly recommend a follow-up review though :)