Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Jun 20, 2019

The Problem

  1. When shutting down the application with an INT signal (Ctrl-C), the database is not flushed. Nevertheless, it will be automatically flushed when starting the application again, but this is not desirable since we are forcing users to always start the application just to persist pending changes that couldn't be applied when the application stopped.

  2. If we flush the database before shutting down, we receive a pure virtual method called in some cases. The error means we must drop the database handle before static destruction kicks in.

Solution to 1

The solution here is very simple, when handling the shutdown signal (impl Handler<Shutdown> for Controller) we issue a Stop message to the App actor, and in turn this one sends a Flush message to the Storage actor.

Solution to 2

Solution 1 is very simple, but after we implement it we find ourselves with issue 2. The reason is this line in Storage's actor builder:

let db_ref = Arc::new(db);
let addr = SyncArbiter::start(1, move || Storage::new(db_ref.clone(), ...));

The reasons for doing this are:

  • The closure SyncArbiter::start receives has type Fn() -> impl Actor
  • We want to create the db instance outside that closure to be able to notify the user with an error if the database cannot be opened (we could panic from inside the Storage actor but that's not the ideal solution I'm after)
  • We then use an Arc to guard the access to the db instance
  • This Arc is captured by the closure
  • Then, when we stop the actor system, it seems that the sync arbiter thread is unable to cleanup before the main thread exists, causing the issue 2

The solution I've come with is to keep the db ownership inside the App actor itself, and whenever a storage-related message is sent to the Storage actor, we send with it a reference to the db. The difference with the previous solution is that now the references sent to the Storage actor are only "alive" during the message handling, and not for the entire lifetime of the Storage's sync-arbiter thread.

@ghost ghost force-pushed the system-shutdown branch from e4238b7 to 68b74a5 Compare June 20, 2019 14:33
Copy link
Contributor

@tmpolaczyk tmpolaczyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to work

@ghost ghost merged commit 68b74a5 into witnet:master Jun 20, 2019
@ghost ghost deleted the system-shutdown branch June 20, 2019 20:42
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant