Register run on stop callbacks #2402

nimmaj · 2018-01-21T10:33:51Z

👮🏻👮🏻👮🏻 !!!! DESCRIBE YOUR CHANGES HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻

When writing driver based integration tests in the cordapp-template using Braid, there is a nice shutdown mechanism used by Corda to shut all of its services down without shutting down the VM. Unfortunately there is no mechanism for Braid to know about this so Braid leaves ports open and lying around and the second test is unable to run.

This PR exposes the existing runOnStop infrastructure in AbstractNode to CordaServices via ServiceHub. I initially put it in ServiceHubInternal but during conversation with Joel we moved it to ServiceHub.

I also wrote tests, admittedly driver based, in the cordapp-template, which worked fine.

PR Checklist:

[ Yes] Have you run the unit, integration and smoke tests as described here? https://docs.corda.net/head/testing.html

[Yes ] If you added/changed public APIs, did you write/update the JavaDocs?

Probably need reviewing for consistency/correct nomenclature.

[Yes ] If the changes are of interest to application developers, have you added them to the changelog, and potentially release notes?
[X ] If you are contributing for the first time, please read the agreement in CONTRIBUTING.md now and add to this Pull Request that you agree to it.

I agree to the current contents of CONTRIBUTING.md.

Look forward to your feedback.

Thanks, Ben

Thanks for your code, it's appreciated! :)

mikehearn

Thanks for the contribution!

I've done a review of the current design below. @mnesbit has in the past mentioned that maybe we should be exposing more sophisticated lifecycle management to the service objects. For example, to prepare for a future where we can start and stop apps without starting and stopping the node itself.

mikehearn · 2018-01-22T11:06:43Z

core/src/main/kotlin/net/corda/core/node/ServiceHub.kt

+     *
+     * You should not rely on this to clean up executing flows - that's what quasar is for.
+     */
+    fun addRunOnStop(runOnStop: () -> Any?)


Maybe registerUnloadHandler is a better name? Again, because that way we can change when the callback is used in future?

Also - rather than use Any? which implies the result might be used for something, why not use Unit. Unit is a special type in Kotlin which is used in place of void in Java. So it will be translated to () -> void which is a more helpful API for Java users and people using other JVM languages. I realise the internal list uses Any? because that way we can jam any function at all into it, but for public API we should be more precise in our types. I think you can assign () -> Unit to () -> Any in Kotlin so it should involve no code changes on your part.

mikehearn · 2018-01-22T11:08:41Z

core/src/main/kotlin/net/corda/core/node/ServiceHub.kt

@@ -354,4 +354,13 @@ interface ServiceHub : ServicesForResolution {
     * @return A new [Connection]
     */
    fun jdbcSession(): Connection
+
+    /**
+     * Allows the registration of a callback to inform services when corda is shutting down.


This is a good api doc, but I'd add the following paragraph:

"Please note that the shutdown handler is not guaranteed to be called. In production the node process may crash, be killed by the operating system and other forms of fatal termination may occur that result in this code never running. So you should use this functionality only for unit/integration testing or for code that can optimise the shutdown e.g. by cleaning up things that would otherwise trigger a slow recovery process next time the node starts."

I would also tweak the text so it talks about shutdown of the app not shutdown of the node. Subtle distinction without a difference in the current platform, but it ensures that we are not breaking semantic compatibility later if we start running this callback when an app is unloaded on the fly.

mikehearn · 2018-01-22T11:15:41Z

node/src/integration-test/kotlin/net/corda/node/NodeTests.kt

+import java.util.concurrent.CountDownLatch
+import java.util.concurrent.TimeUnit
+
+private fun checkQuasarAgent() {


I realise that this is here because you encountered this problem but it's something that should be asserted inside the driver code. Can you take this out and/or move it into the implementation of the driver DSL? The idea is right, it's just the location that's wrong.

mikehearn · 2018-01-22T11:16:07Z

node/src/integration-test/kotlin/net/corda/node/NodeTests.kt

+    }
+}
+
+val latch = CountDownLatch(1)


This can be moved into the companion object - at the top level it'll pollute the global namespace, interfering with code completion and slowing down the IDE.

mikehearn · 2018-01-22T11:26:53Z

node/src/integration-test/kotlin/net/corda/node/NodeTests.kt

+        checkQuasarAgent()
+    }
+
+    @Test


Isn't there an existing test file that this can be put in? NodeTests is an awfully generic name for a class that only has a single unit test in it - again, this class will show up in code completion and navigation, so it's good for it to have a specific name if it can't be merged with some other test suite.

mikehearn · 2018-01-22T11:27:08Z

testing/node-driver/src/main/kotlin/net/corda/testing/node/MockServices.kt

@@ -57,6 +57,8 @@ open class MockServices private constructor(
        private val initialIdentity: TestIdentity,
        private val moreKeys: Array<out KeyPair>
 ) : ServiceHub, StateLoader by validatedTransactions {
+


No need for these extra blank lines.

nimmaj · 2018-01-22T13:04:03Z

Awesome comments - thanks for those. I believe I've made all the requested updates. The only contentious change is that I've renamed NodeTests rather than merged into another one. The obvious candidate to merge with is NodeKeyStoreCheckTest, but merging those two together is likely to create a suite with the name NodeTests again, arguably missing the point of the comment. I originally called my test NodeTests because I was worried about adding my test to one of the existing named tests! Do let me know your thoughts around this. Thanks for taking the time to review.

mikehearn

Thanks, that looks a lot better. I noticed one minor thing that doesn't matter much, and I asked Andras to comment on the Quasar check in Driver DSL, as he authored the code originally. After that I think we're there.

mikehearn · 2018-01-22T16:12:18Z

node/src/integration-test/kotlin/net/corda/node/NodeUnloadHandlerTests.kt

+    class RunOnStopTestService(serviceHub: ServiceHub) : SingletonSerializeAsToken() {
+
+        companion object {
+            private val log = loggerFor<RunOnStopTestService>()


You can use contextLogger() here - it's shorter and survives moves/renames.

That's much nicer. Thanks :-)

mikehearn · 2018-01-22T16:13:36Z

testing/node-driver/src/main/kotlin/net/corda/testing/node/internal/DriverDSLImpl.kt

@@ -888,6 +889,9 @@ fun <DI : DriverDSL, D : InternalDriverDSL, A> genericDriver(
    val serializationEnv = setGlobalSerialization(initialiseSerialization)
    val shutdownHook = addShutdownHook(driverDsl::shutdown)
    try {
+        if (!(ManagementFactory.getRuntimeMXBean().inputArguments.any { it.contains("quasar") })) {


@exFalso Does this place look right to you? I suspect we should only check for this, if in-memory mode is in use - concur?

That's interesting because where I've put it, I found I needed to add a line to the build.gradle to make the web server integration tests pass (they're broken in the current version of this PR - apols for that - i didn't realise that pushing to my repo would immediately build here). So I would be interested in your views.

@mikehearn out-of-process nodes have this argument specified explicitly elsewhere so the spawned jvms won't use the argument that's checked here. This code works anyway because we specify the agent for all tests.
Perhaps move this to startInProcessNode() to make it clear that it's a precondition of starting in-process nodes. This will delay the failure until a startNode is called, but I don't think that's an issue

nimmaj · 2018-01-22T17:28:47Z

Thanks. Checked in a fix for the breaking test (that may turn out to be irrelevant) and your suggested logging change which is rather nice. Tests seem to pass locally, though i'm struggling with dangling JVMs - don't know if that's normal?

exFalso

Small things, otherwise LGTM. Neat PR!

exFalso · 2018-01-23T15:05:42Z

testing/node-driver/src/main/kotlin/net/corda/testing/node/internal/DriverDSLImpl.kt

@@ -888,6 +889,9 @@ fun <DI : DriverDSL, D : InternalDriverDSL, A> genericDriver(
    val serializationEnv = setGlobalSerialization(initialiseSerialization)
    val shutdownHook = addShutdownHook(driverDsl::shutdown)
    try {
+        if (!(ManagementFactory.getRuntimeMXBean().inputArguments.any { it.contains("quasar") })) {


@mikehearn out-of-process nodes have this argument specified explicitly elsewhere so the spawned jvms won't use the argument that's checked here. This code works anyway because we specify the agent for all tests.
Perhaps move this to startInProcessNode() to make it clear that it's a precondition of starting in-process nodes. This will delay the failure until a startNode is called, but I don't think that's an issue

exFalso · 2018-01-23T15:13:18Z

node/src/integration-test/kotlin/net/corda/node/NodePerformanceTests.kt

@@ -31,13 +31,6 @@ import java.util.*
 import java.util.concurrent.TimeUnit
 import kotlin.streams.toList

-
-private fun checkQuasarAgent() {
-    if (!(ManagementFactory.getRuntimeMXBean().inputArguments.any { it.contains("quasar") })) {


Was about to comment that this is already done here when I noticed you updated this code as well! Thorough:)

There is one more test that replicates this behaviour, NodeStatePersistenceTests. There this check is done to decide whether to run nodes in or out of process. I believe this is incorrect and may have been left in from a debugging session. Branching on this jvm flag means that running the test from IntelliJ may result in different behaviour than running it from gradle, so I think that code can simply be removed, it should always be out-of-process.

exFalso · 2018-01-23T15:21:43Z

webserver/build.gradle

@@ -1,5 +1,6 @@
 apply plugin: 'kotlin'
 apply plugin: 'java'
+apply plugin: 'net.corda.plugins.quasar-utils'


If we move the check to startInProcessNode this will perhaps not be needed anymore

@exFalso

…sted by @exFalso

nimmaj · 2018-01-24T14:03:30Z

Sorry for the delay - machine was being fixed! Ok, I've moved the code as suggested and reverted the build.gradle - feels better now - thanks for the suggestion :-)

mikehearn · 2018-01-24T14:49:32Z

Thanks and no need to apologise for any delays, you're the one contributing and we're the one making demands here :)

It looks good to me so I approved it. Once it passes integration testing I'll merge. Thanks for the help!

Ben Wyeth added 2 commits January 19, 2018 13:09

initial commit - all tests pass locally

b39be40

adding changelog entry as per PR instructions

e3e09f2

nimmaj changed the title ~~Nimmaj register run on stop~~ Register run on stop callbacks Jan 21, 2018

mikehearn suggested changes Jan 22, 2018

View reviewed changes

addressing excellent PR comments

10fe3eb

mikehearn reviewed Jan 22, 2018

View reviewed changes

test fix and logging PR comment fix

203d7ea

exFalso reviewed Jan 23, 2018

View reviewed changes

moving the quasar check to a slightly more sensible location as sugge…

6b80304

…sted by @exFalso

mikehearn approved these changes Jan 24, 2018

View reviewed changes

mikehearn merged commit d17670c into corda:master Jan 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Register run on stop callbacks #2402

Register run on stop callbacks #2402

nimmaj commented Jan 21, 2018

mikehearn left a comment

mikehearn Jan 22, 2018

mikehearn Jan 22, 2018

mikehearn Jan 22, 2018

mikehearn Jan 22, 2018

mikehearn Jan 22, 2018

mikehearn Jan 22, 2018

nimmaj commented Jan 22, 2018

mikehearn left a comment

mikehearn Jan 22, 2018

nimmaj Jan 22, 2018

mikehearn Jan 22, 2018

nimmaj Jan 22, 2018

exFalso Jan 23, 2018

nimmaj commented Jan 22, 2018

exFalso left a comment

exFalso Jan 23, 2018

exFalso Jan 23, 2018

exFalso Jan 23, 2018

nimmaj commented Jan 24, 2018

mikehearn commented Jan 24, 2018

Register run on stop callbacks #2402

Register run on stop callbacks #2402

Conversation

nimmaj commented Jan 21, 2018

PR Checklist:

mikehearn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nimmaj commented Jan 22, 2018

mikehearn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nimmaj commented Jan 22, 2018

exFalso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nimmaj commented Jan 24, 2018

mikehearn commented Jan 24, 2018