Is there a way to install a cleanup callback/hook when the server shutdowns or the dev build reloads the application?

[zypus]

I'm currently trying to use xodus in combination with kobweb. However, I wasn't able to find a way to free up the acquired xodus instance when the dev build rebuilds the server. Therefore once the dev rebuild is done the xodus connection cannot be reacquired, because the instance wasn't closed properly (basically a form of file locking occurs). Shutting down kobweb and rerunning does work though.

Is there any functionality available in kobweb, which would allow to free up resource on shutdown/rebuild, I am not aware off?

If not, I guess it would be convenient to have the analog to the @ApiInit annotation, maybe @ApiCleanup that gets called when the server rebuilds or get shutdown.

[bitspittle]

Great question.

The three places I can think to put cleanup logic are

1. an @ApiCleanup method as you suggested
2. expose a cleanup hook in the Kobweb Server plugin API (read more here: https://github.com/varabyte/kobweb#kobweb-server-plugins). Can you read that section and let me know what you think about that?
3. I suspect you could do something right now in Gradle somehow, listening for build events and releasing the instance on some event. Maybe you can register a task that gets called right before kobwebStart ? Or maybe I can expose some Kobweb API for your build.gradle where you can register a callback that gets triggered on server start?

Can you share a bit with me how you'd ideally release the instance? Is it something you'd rather do outside the server code (so, Gradle) or should it be inside the server code as it needs to have access to some data value in there (e.g. the inverse of something you set in an @ApiInit method)

[zypus]

My current setup looks like the following:

val store = PersistentEntityStores.newInstance(".xodus-data")

@InitApi
fun initDatabase(ctx: InitApiContext) {
    val addressRepo = AddressRepositoryImpl(store, ctx.logger)
    val personRepo = PersonRepositoryImpl(store, addressRepo, ctx.logger)
    val userRepo = UserRepositoryImpl(store, personRepo, ctx.logger)

    ctx.data.add<AddressRepository>(addressRepo)
    ctx.data.add<PersonRepository>(personRepo)
    ctx.data.add<UserRepository>(userRepo)
}

So I believe what needs to happen is that store.close() needs to be called before the api.jar is reloaded by the server. I am not sure that can be achieved by adding a server plugin or by using a gradle callback. As xodus is an embedded database, ie. in the current setup it will be part of the api.jar.

So as you mentioned it probably needs to work in an inverse fashion to the @InitApi. I believe when in the ApiJarFile the cache gets invalidated a function of the current ApiFactoryImpl should be called that handles the cleanup.

So for the above a callback could be registered like so:

@DisposeApi
fun releaseDatabase() {
    store.close()
}

I will edit the kobweb library locally to see if that would solve my current problem and will create a pull-request so you can decide if that is a feasible approach.

[zypus]

I know tested the proposal from above and it would work splendidly for my use case. I can still use my xodus database even after the dev server reloads the api.jar.

I made a pull request #352 with a preliminary implementation, that currently only calls the dispose callback for the case when the dev server reloads the jar, but not when the server shutdown. I felt those changes would need to be a bit bigger, than what I have added so I left this part open.

[bitspittle]

Thanks for the pull request! And the code example. Please give me a day to process it / think about it / read up on xodus a bit. Based on your code example, this is probably the right way. If this is going to go in, I'll aim to get it into the next release, 0.14.2, which is probably a few days away.

The only thing I'm worried about is conveying to users that it's not guaranteed the dispose call will get triggered. If a server crashes, for example, or the machine it's running on is shut down forcefully, then your server might go away without the dispose call ever getting called. Does this imply that people should be checking for and handling leftover resources in the init methods?

Is that a possibility for you? That you can check for the existence of the ".xodus-data" file inside an init function instead? If you were running Kobweb on a server and your process got force killed and restarted, would xodus work in that case or would things be broken because of a leftover lock file?

If you've ever written code in C#, the language provides a dispose destructor which you should pretty much never use because it's so unreliable. I basically want to make sure this won't be a feature like that.

Other than that, I think @DisposeApi is a good name. I would say we should add ctx: DisposeApiContext even if all it has is the data field at this point. You're using a global in your case, which is fine, but I can imagine someone might store their disposable resource in the data field only to clear it up in the dispose method.

[zypus]

Thank you.

You are right, I am also not entirely happy with such a dispose function not being called in all cases. However, in my case this does not seem to matter, if the server crashes or is killed the unreleased lock does not take hold at the next startup. I read a bit about the xodus internals and apparently the actual existence of the lock file (in my case .xodus-data/xd.lck) does not matter. Somehow the JVM keeps track of the locks on these files internally. So it only matters if there are two processes or multiple calls from the JVM attempting to lock the same file. As of now, I am not sure there is a way for me to forcefully release this internal JVM lock during initalisation instead.

And I totally agree with your last point, that if we keep the dispose functionality, we should provide a ctx: DisposeApiContext with the data. I just implemented the minimal requirement as it was already late and I quickly wanted to test if that would help at all. Originally I was storing the database store also in ctx.data.

[bitspittle]

Good morning. I just woke up, and after sleeping on it, I think this is worth putting in, even if it needs to be documented carefully. As you point out in your case, it can at least be useful for development.

I also thought we might not even need to create a new dispose annotation if we just pass an extra field into the init context. Imagine code like this:

@InitApi
fun initStuff(ctx) {
   // Option 1 - another ctx property
   ctx.onDispose { ... }
   // Option 2 - add to data 
   ctx.data.onDispose { ... }
   // Option 3 - create new events property 
   // Only one event so far but maybe more later?
   ctx.events.onDispose { ... }
}

I'm not suggesting you change your PR, I'm just thinking out loud and curious what you think about this approach overall and the specific options.

[zypus]

Yes, currently I only need this feature for development anyway, because this shouldn't be needed once deployed (in my specific usecase only of cause). But yeah the caveats should be pointed out explicitly in the documentation.

I quite like your idea, this also reminds me of the DisposableEffect used in Jetpack Compose:

DisposableEffect(key1 = viewModel) {
      viewModel.startLogging()
      onDispose {
          viewModel.stopAndLogTime()
      }
 }

I like Option 1 and Option 3, because both options indicate more that the whole ctx will be disposes/replaced. In my opinion, Option 2 would pollute the Data class, by giving it another responsibility than just holding data.

I slightly prefer Option 3 over 1, because as you mentioned it would be cleaner to extend in case there are other events that might be needed in the future.

[bitspittle]

Option 3 is my favorite too. Did you want to take a stab at updating your PR / adding a new PR? You can also kick it to me (I'll set you as the author) since I appreciate your help so far and if you might be busy for the next few days.

[zypus]

I'll have some time today. I can try updating my PR based on what we have discussed. I let you know in case I run into problems.

[zypus]

Please have a look at the new pull request #353. What do you think about the implementation?

[bitspittle]

Thank you so much, I will take a look shortly and let you know.