InitializePhase2() and CleanUp() methods are never called
Closed this issue · 4 comments
InitializePhase2() and CleanUp() methods should be defined as virtual in ExtensionMain so that extensions can implement these methods for additional processing. The immediate need is for the Replicator widget to be able to run v3.0 of the harvest extensions. The harvest extensions need to deinitialize the site-harvest extension at the end of each scenario run so that it can be reinitialized for the succeeding run(s).
This method should be available for all extensions as it is good practice to clean-up resources,such as logs, when a scenario finishes running. It isn't currently an issue with the command line interface because all processes end after each scenario but pose future problems if the scenarios are run through a different UI.
The risk related to this implementation is that extensions with InitializePhase2() and CleanUp() methods that have not been running will start running.
This issue needs to be resolved before LANDIS-II-Foundation/Tools-Archive#3 and LANDIS-II-Foundation/Extension-Base-Harvest#5 can be tackled.
Input from Jimm Domingo 3-NOV-2015:
I'm not sure the methods would start to run. If a developer had written CleanUp or InitializePhase2 methods in an extension, she would've had to use the "new" keyword to hide the non-virtual counterparts in the ExtensionMain base class. Otherwise, the compiler would complain. So she would've silenced the compiler even though the core couldn't access the methods in the extension's Main class.
If those 2 methods are made virtual in the ExtensionMain, then extension developers have to use the "override" keyword. So if they had written CleanUp or InitializePhase2 methods, they'd have to change their signatures. IOW, they'd need to update the source code. So it's not a source-compatible change. I don't know if changing the signatures of the methods in ExtensionMain is binary-compatible.
If it's not (i.e., it's binary-incompatible), the core wouldn't be able load any extensions with those definitions of those methods. If it is a binary-compatible change, the core may be able to load those type of extensions, but it may not be able to call those methods because the methods are defined with the "new" keyword. My C# is a bit rusty of late, but I don't think methods defined that way are accessible via the base class (which is how the core accesses all the exension's Main classes -- through the ExtensionMain base class). That'll need to be checked out further with some test code.
This change (making those methods virtual) is looking like it may be an API-breaking change. And such a change means a bump in the major version (L-II 7.0). It'd be a lot of work, but it may be worth surveying the source code of existing extensions to see if any of them define a CleanUp or InitializePhase2 methods.
Made the change to ExtensionMain.cs on the Widgets branch. With accompanying changes to the Site Harvest Lib and Base Harvest Extension, it solves the problem we are trying to solve. However, given Jimm's comments, we should probably push the version to at least 6.2 which requires additional code changes. Waiting to get approval from TAC before proceeding further.
Summary for TAC:
The ExtensionMain class is the base class for all extensions. The model core controls the extensions using methods and properties that defined in the ExtensionMain class. There are two methods in ExtensionMain defined incorrectly so that they cannot be implemented/defined in an extension. These methods are InitializePhase2() and CleanUp(). These methods are executed by the model core but nothing happens because they cannot be implemented/defined in an extension.
InitializePhase2() is called after all extensions have initialized potentially allowing additional initialization functionality that relies on the initialization of an extension. CleanUp() is called after all timesteps of a scenario have run. This allows an extension to clean up after itself by releasing resources such as "handles" to log files. If a handles is open on a log file, no other processes or programs can access it.
We recommend that the definition of these two methods be changed from void to virtual allowing extensions to "override" these methods and implement functionality within them. We have been told that this was the original intention for these methods but that an error was made when this code was refactored in the past. We also recommend that the core version be updated from 6.1 to 6.2 as this is a significant change.
The immediate need for this change is to allow scenarios to be run sequentially using the "Widgets" Replicator or Launcher. Since the current implementation doesn't support the CleanUp() method, many extensions hold open resources so the scenario cannot run a second time. This is not a problem when running LANDIS-II from the console because resources are automatically cleared at the end of a scenario. This is inherent with how the console works but Windows applications (widgets) control memory differently. This could also pose a problem in the future when running LANDIS-II on the cloud depending on how the application is run.
See Jimm Domingo's comment from NOV 3 on the risk involved in making this change. We recommend that it be reviewed by one or more experienced C# developers in the LANDIS-II community and also that a repository search be done (if possible) for the use of InitializePhase2() and CleanUp() methods in existing extension code.
ready to close