Update java/Ice/greeter shutdown #332
Conversation
|
|
||
| package com.zeroc.demos.greeter; | ||
|
|
||
| import com.zeroc.demos.greeter.Chatbot; |
There was a problem hiding this comment.
Don't need import since Chatbot is in the same package.
| } | ||
| } | ||
|
|
||
| private static void shutdownCommunicatorOnCtrlC(Communicator communicator, Thread cleanupThread) { |
There was a problem hiding this comment.
I don't like cleanupThread name very much, what about using just mainThread?
There was a problem hiding this comment.
I also prefer mainThread.
It's not obvious from the name cleanupThread that your intention is that a user would do cleanup in the main thread, since we don't do any. It also seems to go against the convention in Java that you would do the cleanup in this hook.
There was a problem hiding this comment.
since we don't do any.
We do perform cleanup in this thread: the cleanup is the destruction of the communicator that occurs at the end of the try-with-resources statement.
It also seems to go against the convention in Java that you would do the cleanup in this hook.
What do you suggest then?
This PR is all about NOT performing the cleanup in the hook, but waiting in the hook for some other thread (the cleanup thread) to complete the cleanup.
There was a problem hiding this comment.
Like Jose suggested, I would just call it mainThread.
| } | ||
| } | ||
|
|
||
| private static void shutdownCommunicatorOnCtrlC(Communicator communicator, Thread cleanupThread) { |
There was a problem hiding this comment.
I also prefer mainThread.
It's not obvious from the name cleanupThread that your intention is that a user would do cleanup in the main thread, since we don't do any. It also seems to go against the convention in Java that you would do the cleanup in this hook.
| ### Windows | ||
|
|
||
| ```shell | ||
| server\build\install\server\binserver |
There was a problem hiding this comment.
Just to answer your question, this is the default location where the application plugin we use puts these executables. Maybe we could tweak it, but this is the default.
If you're a normal Java developer, used to gradle, what you would expect to do is this:
./gradlew :server:run
./gradlew :client:run
And this does work, but it also outputs extra clutter which obscures what the demo is actually doing.
You can remove this clutter by running:
./gradlew :server:run --quiet
./gradlew :client:run --quiet
Which I think is slightly clearer than having to type out the paths directly.
So, I think we should switch to recommending these commands instead.
There was a problem hiding this comment.
I think we change this because how Ctrl-C is handled by the scripts.
4 participants
This PR update the shutdown of the java/Ice/greeter demo to be more like the shutdown of demos in other languages.