Some suggestions abount BusConfiguration and @Handler.

[bobwenx]

#1:

net.engio.mbassy.bus.BusConfiguration has only one default constructor:

    public BusConfiguration() {
        super();
        this.numberOfMessageDispatchers = 2;
        this.maximumNumberOfPendingMessages = Integer.MAX_VALUE;
        this.executor = new ThreadPoolExecutor(10, 10, 1, TimeUnit.MINUTES, new LinkedBlockingQueue<Runnable>(), DaemonThreadFactory);
    }

so, when i construct it and i want to set it's executor to another executor, i have to first get the default one and shutdown it. the code is tedious:

        BusConfiguration eventBusConfig = new BusConfiguration();
        // used in synchronous environment only!
        eventBusConfig.setMaximumNumberOfPendingMessages(1);
        eventBusConfig.setNumberOfMessageDispatchers(0);

        ExecutorService defaultExecutor = eventBusConfig.getExecutor();
        defaultExecutor.shutdown();
        try {
            defaultExecutor.awaitTermination(1, TimeUnit.MINUTES);
        }
        catch (InterruptedException e) {
            logger.error("Fail to closed default executor service!", e);
            // ignored
        }
        eventBusConfig.setExecutor(new ThreadPoolExecutor(1, 1,
                                                          0L, TimeUnit.MILLISECONDS,
                                                          new LinkedBlockingQueue<Runnable>(), new ThreadFactory()
        {
            @Override
            public Thread newThread(Runnable r) {
                Thread thread = Executors.defaultThreadFactory().newThread(r);
                thread.setDaemon(true);
                return thread;
            }
        }));
        eventBusConfig.setMetadataReader(new SubscribeConfigChangedMedataReader());
        this.eventBus = new MBassador<>(eventBusConfig);

may be it need more suitable design, for example: move the code in constructor to Default() method:

 public static BusConfiguration Default() {
      BusConfiguration defaultConfig =  new BusConfiguration();
      // initialize default value
     return defaultConfig;
 } 

#2:

the test case: src\test\java\org\mbassy\ConcurrentSetTest.java has a method called testIteratorCleanup():

    @Test
    public void testIteratorCleanup(){
        final HashSet<Object> persistingCandidates = new HashSet<Object>();
        final ConcurrentSet<Object> testSet = new ConcurrentSet<Object>();
        Random rand = new Random();

        for (int i = 0; i < numberOfElements; i++) {
            Object candidate = new Object();

            if (rand.nextInt() % 3 == 0) {
                persistingCandidates.add(candidate);
            }
            testSet.add(candidate);
        }

        // this will remove all objects that have not been inserted into the set of persisting candidates
        runGC();


        ConcurrentExecutor.runConcurrent(new Runnable() {
            @Override
            public void run() {
                for (Object testObject : testSet) {
                    // do nothing
                    // just iterate to trigger automatic clean up
                    System.currentTimeMillis();
                }
            }
        }, numberOfThreads);

        assertEquals(persistingCandidates.size(), testSet.size());
        for (Object test : testSet) {
            assertTrue(persistingCandidates.contains(test));
        }
    }

in my machine(windows 8 x64 + jdk 1.7_10), this test case will fail due to GC threads is running slowly. i have to add some time-pause to get around it.

    @Test
    public void testIteratorCleanup() throws Exception{
        final HashSet<Object> persistingCandidates = new HashSet<Object>();
        final ConcurrentSet<Object> testSet = new ConcurrentSet<Object>();
        Random rand = new Random();

        for (int i = 0; i < numberOfElements; i++) {
            Object candidate = new Object();

            if (rand.nextInt() % 3 == 0) {
                persistingCandidates.add(candidate);
            }
            testSet.add(candidate);
        }

        // this will remove all objects that have not been inserted into the set of persisting candidates
        runGC();
        // add this to prevent some case that GC Threads are processed too slow.
        Thread.sleep(2000);

        ConcurrentExecutor.runConcurrent(new Runnable() {
            @Override
            public void run() {
                for (Object testObject : testSet) {
                    // do nothing
                    // just iterate to trigger automatic clean up
                    System.currentTimeMillis();
                }
            }
        }, numberOfThreads);

        assertEquals(persistingCandidates.size(), testSet.size());
        for (Object test : testSet) {
            assertTrue(persistingCandidates.contains(test));
        }
    }

#3

i think it may be a good idea to add Custome annotation that can subscribe the event. for example, change @handle's defination, let it can annotate to another Custome annotation that can subscribe event:

@Retention(value = RetentionPolicy.RUNTIME)
@Inherited
@Target(value = {ElementType.METHOD,ElementType.ANNOTATION_TYPE})
public @interface Handler {}

then, i can define my custome event handle annotation:

@Retention(value = RetentionPolicy.RUNTIME)
@Inherited
@Target(value = {ElementType.METHOD})
@Handle
public @interface ConfigChangedEventHandle

[durron597]

Hi bobwenx, if you want a synchronous environment only, it's actually worse than you think:

#45

However, most of that doesn't matter all that much if you use the synchronous dispatch only. (i.e. .publish() or post().now(). Also, a lot of your code in your example is used to create the replacement ExecutorService, which you would have to do anyway if you wanted to use one that isn't the default. That said, your point is valid, that's why I made #45 for bennidi to look at.

Also, you may want to consider splitting this into three separate issues, so bennidi can address them all separately.

[durron597]

Also, you pasted this method call:

eventBusConfig.setMetadataReader(new SubscribeConfigChangedMedataReader());

You may want to remove that, if you subclassed BusConfiguration. But I bet you changed a lot more than that, to get that method to work :)

[bobwenx]

hi, durron597:
the code:

eventBusConfig.setMetadataReader(new SubscribeConfigChangedMedataReader());

has only purpose for process custome annotation, as i suggestion in above.
And it does't change a lot code, just the parse logical of @handle annotation.

for BusConfiguration class, i think there may have different factory method for various environment, for example:

 public static BusConfiguration Default() {
      BusConfiguration defaultConfig =  new BusConfiguration();
      // initialize default value for both sync & async environment.
     return defaultConfig;
 } 

Or

 public static BusConfiguration SyncConfiguration() {
      BusConfiguration defaultConfig =  new BusConfiguration();
      // initialize default value for sync environment only.
     return defaultConfig;
 } 

[bennidi]

Hey, just a short update. I am almost done integrating part of your suggestions. This will contain a restructuring of the BusConfiguration using an additional interface and adding thread factories such that the asynchronous parts can be customized. I will also add factory methods for different scenarios (sync[guaranteed sequential processing], async). I restructured some of the code internally and fixed the dead message bug #44. I did not really understand the benefit of the custom handler annotation, though...

[bennidi]

Hey, just another note: I changed the way how a MessageHandler is constructed such that it is now easily possible to provide necessary handler information from different sources (annotations) than the standard mbassador way of doing things. I also added a factory for bus instances which I plan to extend with different scenarios and bus types. Currently it contains a scenario where asynchronously dispatched messages are guaranteed to be processed in their order of dispatch. I hope that makes you happy. Cheers!