Standard Plugin - Sanity check please

[tmiranda]

I've just started to use the "Standard" plugin type and before I get too far along I'd like a sanity check that I am doing things the right way. Can you please comment on this code and let me know if I am doing something stupid, dangerous or just plain wrong?

This stub is just supposed to handle the start(), stop() and destroy() methods. start() should start a thread that will run in the background, stop() should stop the background thread and destroy() should clean up things.

I can compile it, install it and run it and it appears to do what it's supposed to, but I am just learning java and probably have overlooked something.

Does destroy() do the right things? My understanding is that if I set the object references to null the GC will clean things up as needed.

Is it OK to use the public statics in the plugin class?

package tmiranda.mob;

import java.lang.Thread;

/**
 *
 * @author Tom Miranda
 */
public class plugin implements sage.SageTVPlugin {

        public static Thread recordmanagerthread;
        public static recordmanager t;

        public plugin(sage.SageTVPluginRegistry registry) {
            // Do nothing for now.
        }

        public static void startRecordManager() {
            t = new recordmanager();
            recordmanagerthread = new Thread(t);
            recordmanagerthread.start();
        }

	// This method is called when the plugin should startup
	public void start() {
            System.out.println("*** start ***");
            startRecordManager();
        }

	// This method is called when the plugin should shutdown
	public void stop() {
            System.out.println("*** stop ***");
            recordmanagerthread.interrupt();
        }

	// This method is called after plugin shutdown to free any resources
	// used by the plugin
	public void destroy() {
            System.out.println("*** destroy ***");
            recordmanagerthread = null;
            t = null;
        }

	// Returns the names of the settings for this plugin
	public String[] getConfigSettings() {
            System.out.println("*** getConfigSettings ***");
            return null;
        }

	// Returns the current value of the specified setting for this plugin
	public String getConfigValue(String setting) {
            System.out.println("*** getConfigValue ***");
            return null;
        }

	// Returns the current value of the specified multichoice setting for
	// this plugin
	public String[] getConfigValues(String setting) {
            System.out.println("*** getConfigValues ***");
            return null;
        }

	// Returns one of the constants above that indicates what type of value
	// is used for a specific settings
	public int getConfigType(String setting) {
            System.out.println("*** getConfigTyoe ***");
            return 0;
        }

	// Sets a configuration value for this plugin
	public void setConfigValue(String setting, String value) {
            System.out.println("*** setConfigValue ***");
        }

	// Sets a configuration values for this plugin for a multiselect choice
	public void setConfigValues(String setting, String[] values) {
            System.out.println("*** setConfigValues ***");
        }

	// For CONFIG_CHOICE settings; this returns the list of choices
	public String[] getConfigOptions(String setting) {
            System.out.println("*** getConfigOptions ***");
            return null;
        }

	// Returns the help text for a configuration setting
	public String getConfigHelpText(String setting) {
            return null;
        }

	// Returns the label used to present this setting to the user
	public String getConfigLabel(String setting) {
            System.out.println("*** getConfigLabel ***");
            return null;
        }

	// Resets the configuration of this plugin
	public void resetConfig() {
            System.out.println("*** resetConfig ***");
        }

	public void sageEvent(String eventName, java.util.Map eventVars) {
            System.out.println("*** sageEvent ***");
        }

}

package tmiranda.mob;

import sagex.api.*;

/**
 *
 * @author Tom Miranda
 */
public class recordmanager implements Runnable {

    public void run() {
        Global.DebugLog("SRM: Starting SageRecordManager."); // See if I can call DebugLog()

        while(true) {
            
            try {
                System.out.println("SRM: Waking up...");
                Thread.sleep(300000); // Sleep for 5 mins
            } catch(InterruptedException e) {
                System.out.println("SRM: Interrupted.  Terminating thread.");

                // Make sure outer loops also get the signal.
                Thread.currentThread().interrupt();

                break; // Exit the while(true) loop.
            }
        }

    }
}

[jphipps]

Everything looks good, except the statics. You shouldn't need to define those as static if they are all used within the plugin. If you are trying to access those objects from outside the plugin, I would probably define a static method that would return the reference to the class..

Thanks,
Jeff

[tmiranda]

Thanks Jeff. It will only be used in the plugin so I'll drop the "static".

[PLUCKYHD]

Is there not any kind of listener you could subscribe to for this event?

If not looks okay to me as well.

[jreichen]

I noticed a few things

• If your startRecordManager() method will only be called inside that class then it should be private, not public. It doesn't need to be static either.
• I'm not sure what the purpose of the call to interrupt() is in the catch block. But IIRC I think I saw stuckless use that in an example so I'm sure he had a reason.
• There's a timing problem in the code. The InterruptedException is only thrown while the code is in the call to sleep. If you are at some other place in the loop when interrupt() is called, you won't know it and your loop will never exit. You'll need to call Thread.currentThread().interrupted() or Thread.currentThread().isInterrupted() probably either at the beginning of the loop or right before sleep(). See the Thread javadoc for info.

EDIT: The last point is not true, Slugger corrected me below.

[stuckless]

Originally Posted by jreichen

I noticed a few things
[*]I'm not sure what the purpose of the call to interrupt() is in the catch block. But IIRC I think I saw stuckless use that in an example so I'm sure he had a reason.

Maybe this will explain it better than I can... but it is an important step when catching an intteruptedexception.

[*]There's a timing problem in the code. The InterruptedException is only thrown while the code is in the call to sleep. If you are at some other place in the loop when interrupt() is called, you won't know it and your loop will never exit. You'll need to call Thread.currentThread().interrupted() or Thread.currentThread().isInterrupted() probably either at the beginning of the loop or right before sleep(). See the Thread javadoc for info.

These are precisely the reason the why I tend to over emphasize the user of the java Timer and TimerTask classes. Most people really don't do proper thread programming without a lot of extensive knowledge of how threads work. While i've done my share of thread programming over the years, the majority of the "thread" task that i create fall in the 2 main categories...
1. run a single task, once, in the background.
2. run a task every N seconds.

Both of these things fit perfectly into the java Timer/TimerTask model, and you don't have to worry about interrupts, etc. If you need something more advanced, then you'll have to write your own... and deal with the headaches that go with it

[tmiranda]

Thanks for all of your comments.

[stuckless]

While we are talking about Plugins... I'll pitch the AbstractPlugin class from the sagex-api package

If you have your plugin extend this plugin, then you may find it easier to build configuration for your plugin. I wrote this when I first started to use the Plugin interface to simplify the configuration side of things. I have cases where I only want to show some configuration in some conditions, etc.

It also allows your to respsond to configuration change events, but defining a method that you want called when a configuration property changes. this just saves your from having to write code that checks if the property is changing on set, and then act on it.

you can also create methods that you want called when certain Sage Events happen. In some cases you can choose to have those events handled in the background by settings a background flag on the method.

Much of this uses Annotations to pull this off, but the end result, is a much cleaner plugin class, in my oppinion.

You can take a look at the BMT plugin for a sample of how some of the pieces fit together.

That plugin doesn't use the SageEvent annotation but once you get used to the annotations, they are pretty easy to use, and most modern day java IDEs will auto complete them as well.

[PLUCKYHD]

I will back Sean up on that. He sagex implentation is really nice and easy to do.

Although he comment about the IDE auto Completing reminds that I still write way to much code manually.

[jphipps]

I have to second that on the sagex-api for plugins... Makes it very easy to add configuration settings...

I am also liking the Timer class Sean mentions... After seeming him post a lot about it, I tried it out and it is pretty nice....

Thanks,
Jeff

[PLUCKYHD]

Originally Posted by jphipps

I have to second that on the sagex-api for plugins... Makes it very easy to add configuration settings...

I am also liking the Timer class Sean mentions... After seeming him post a lot about it, I tried it out and it is pretty nice....

Thanks,
Jeff

Good to know I am not the only one that gave into his peer pressure and tired it.

[Slugger]

Originally Posted by jreichen

I noticed a few things

• I'm not sure what the purpose of the call to interrupt() is in the catch block. But IIRC I think I saw stuckless use that in an example so I'm sure he had a reason.

In this situation, it's not really necessary to interrupt the thread in the catch block, but it is a very good habit to get into. Basically when the InterruptedException is thrown it clears the interrupted flag in the Thread instance. You interrupt again in the catch block to flip that flag back such that if there were any other loops, etc. wrapped around this thread then they would also be interrupted, allowing the thread to properly terminate. Obviously, that's not the case here, but, as I say, it's a very good habit to get into.

Originally Posted by jreichen

• There's a timing problem in the code. The InterruptedException is only thrown while the code is in the call to sleep. If you are at some other place in the loop when interrupt() is called, you won't know it and your loop will never exit. You'll need to call Thread.currentThread().interrupted() or Thread.currentThread().isInterrupted() probably either at the beginning of the loop or right before sleep(). See the Thread javadoc for info.

This actually isn't true. Thread.sleep() checks the interrupt state before going to sleep and so if the thread was interrupted somewhere else in the while loop it will still interrupt itself at the call to sleep (of course, it will finish what ever it was doing, but once it tries to go to sleep it will check the interrupt flag and immediately head to the catch block instead of sleeping).

With that said, the Java Timer class is usually a nicer approach.

[jreichen]

Originally Posted by Slugger

This actually isn't true. Thread.sleep() checks the interrupt state before going to sleep and so if the thread was interrupted somewhere else in the while loop it will still interrupt itself at the call to sleep (of course, it will finish what ever it was doing, but once it tries to go to sleep it will check the interrupt flag and immediately head to the catch block instead of sleeping)

Thanks for the correction. Now the example Sean posted makes more sense. You saved me the time of running the code

[jreichen]

Add another person to the list of those using AbstractPlugin. It really cleans up your code a lot. However, the Jetty plugin has a lot of interdependencies between properties and conditional help text based on property values so I still ended up writing a fair amount of code in my plugin's subclass. Maybe AbstractPlugin could be enhanced to handle more of it for me, I don't know.