Simple Proposal - Unified listener management

[John Murph]

For our TeamCity-Gradle plugin to report tests correctly, we need to have a way register a test listener.  I want to talk about the test listener later, but right now I want to talk about how one registers a listener and how Gradle deals with that.

Gradle.java now has a method called addBuildListener().  Adam had mentioned the idea of an addListener() method that could take any kind of listener.  I would like to replace addBuildListener() with this method, and register the listener on a ListenerManager object.  Then, any code in Gradle that wants to call back to a listener would go to the ListenerManager and ask for listeners that implement some interface.  This would provide "one-stop shopping" for all listener's in Gradle.  The DefaultGradleFactory could allocate of ListenerManager and inject it into each object in Gradle that needs it.  The only issue I see is that sometimes one want's a different listener to be provided for different times.  In that case, I wonder if the ListenerManager could support ListenerFactory objects that could create new listeners when desired.  I haven't thought that through, though (wow, that's a confusing sentence).

Thoughts?


--
John Murph
Automated Logic Research Team

[Adam Murdoch-2]

John Murph wrote:

> For our TeamCity-Gradle plugin to report tests correctly, we need to
> have a way register a test listener.  I want to talk about the test
> listener later, but right now I want to talk about how one registers a
> listener and how Gradle deals with that.
>
> Gradle.java now has a method called addBuildListener().  Adam had
> mentioned the idea of an addListener() method that could take any kind
> of listener.  I would like to replace addBuildListener() with this
> method, and register the listener on a ListenerManager object.  Then,
> any code in Gradle that wants to call back to a listener would go to
> the ListenerManager and ask for listeners that implement some
> interface.  This would provide "one-stop shopping" for all listener's
> in Gradle.  The DefaultGradleFactory could allocate of ListenerManager
> and inject it into each object in Gradle that needs it.  The only
> issue I see is that sometimes one want's a different listener to be
> provided for different times.  In that case, I wonder if the
> ListenerManager could support ListenerFactory objects that could
> create new listeners when desired.  I haven't thought that through,
> though (wow, that's a confusing sentence).
>
> Thoughts?

I like this idea in general. It would give us a single API behind which
we can deal with cross-cutting event handling things like logging, error
handling and reporting, closure coercion, and concurrency. Later, it
could even deal with remoting - broadcasting and receiving events from
forked and remote Gradle processes.


Adam


---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

[John Murph]

I have some code for this at git://github.com/sappling/gradle.git in branch listener_manager.  I'm not sure if we should integrate this as is, or finish changing over all the other existing listeners to this scheme first.  Next week I will try to get a TestListener submitted that can listen to JUnit (and testng, hopefully) tests.  For that purpose, this code also contains generic listener remoting support in package org.gradle.listener.remote.

Please take a look at it give me some feedback.  I'm especially concerned if the remoting support seems like the right approach (and suggestions for error handling in this code would be nice too).


--
John Murph
Automated Logic Research Team

[Adam Murdoch-2]

John Murph wrote:

I have some code for this at git://github.com/sappling/gradle.git in branch listener_manager.  I'm not sure if we should integrate this as is, or finish changing over all the other existing listeners to this scheme first.  Next week I will try to get a TestListener submitted that can listen to JUnit (and testng, hopefully) tests.  For that purpose, this code also contains generic listener remoting support in package org.gradle.listener.remote.

Please take a look at it give me some feedback.  I'm especially concerned if the remoting support seems like the right approach (and suggestions for error handling in this code would be nice too).

Some comments:

- Generally, I think this is the right direction to take. However, I think it's a little invasive at the moment. More on this below.

- I don't think we should use the Listener marker interface. Requiring it means, for example, we can't use 3rd party listener interfaces. It's just unnecessary coupling. We should be able to broadcast on any interface which has only void methods.

- We should leave the strongly typed addNnnListener() methods, rather than replace them with a single addListener(). We should add addListener() as a convenience only.

Having a single method means we lose the static typing and documentation goodness. It also means we can't have different event streams which use the same interface, such as, GradleLauncher.addStandardOutputListener() and addStandardErrorListener().

I think the only classes which should have an addListener() method are Gradle and GradleLauncher. These are the only places where adding a global listener of a given type make sense.

- I think the changes to LoggingConfigurer are a good example of how the approach is a little too invasive. To me, ListenerManager and LoggingConfigurer are services, and it's always a smell (to me) when a service is passed into another service via the service interface methods. Why should I need a ListenerManager to configure logging? It feels like ListenerManager is leaking into places it doesn't really belong.

Some alternative approaches:

1. Inject the ListenerManager into the DefaultLoggingConfigurer, and leave the addNnnListener() methods on LoggingConfigurer. DefaultLoggingConfigurer uses the ListenerManager to create 2 broadcasters with type StandardOutputListener. That is, it creates 2 event streams. One stream is for standard out, the other for standard error.

2. Move the above logic out of DefaultLoggingConfigurer, and inject the 2 broadcasters into DefaultLoggingConfigurer. Remove the addNnnListener() methods. This way, DefaultLoggingConfigurer know nothing of ListenerManagers. It just knows it needs to emit std out and std err streams.

interface LoggingConfigurer {
    void configure(LogLevel level)
}

class DefaultLoggingConfigurer {
    public DefaultLoggingConfigurer(StandardOutputListener stdoutBroadcaster, StandardOutputListener stderrBroadcaster) {
       ...
   }

   public void configure(LogLevel level) {
      ... attaches the broadcasters to the appropriate appenders ...
   }
}

I think I like option 2.


- DefaultInternalRepository should not register itself. In general, I think it's better for a service to simply declare that it is a listener of some type and have the environment take care of the registering, rather than for the service to know how to register itself. This is just a variation of inversion of control.

- Remote approach looks good, but I would hold off going too far down that path until the native test changes are merged in, because it does pretty much the same stuff.


Adam

[John Murph]

On Mon, Sep 7, 2009 at 6:55 PM, Adam Murdoch <a@...> wrote:

Some comments:

- Generally, I think this is the right direction to take. However, I think it's a little invasive at the moment. More on this below.

- I don't think we should use the Listener marker interface. Requiring it means, for example, we can't use 3rd party listener interfaces. It's just unnecessary coupling. We should be able to broadcast on any interface which has only void methods.

I thought about this as I did it.  I like the stronger typing provided by the Listener interface (although I was uncomfortable with it being a marker interface, that seemed a bad smell).  I would think that Gradle would always want to wrap 3rd party listener interfaces just to isolate itself from changes to those third party libraries, so that didn't seem to be a very important limitation.  (Notice that remoting support doesn't require the Listener interface as it seemed like it would want to use 3rd party interfaces.)  I'm not adamant about this, however, so I'll remove it.
 

- We should leave the strongly typed addNnnListener() methods, rather than replace them with a single addListener(). We should add addListener() as a convenience only.

I don't like two ways of doing the same thing.  Why do we need addNnnListener() methods and a addListener() method on these classes?  That seems unnecessary and confusing.
 

Having a single method means we lose the static typing and documentation goodness. It also means we can't have different event streams which use the same interface, such as, GradleLauncher.addStandardOutputListener() and addStandardErrorListener().

That's one reason for the Listener common base class: it povides static typing and documentation without requiring a rippling of new methods every time a new listener is defined.  It does limit having different even streams that use the same interface, but I don't think that's generally a good idea anyway.  How often does one wish to listen to standard output and not standard error (or vice versa)?  I think a single interface for listening to output makes sense, after all one can implement one of the methods as a "do nothing" just as in every other listener interface which has more than one method.

I think the only classes which should have an addListener() method are Gradle and GradleLauncher. These are the only places where adding a global listener of a given type make sense.

I didn't feel compelled to put an addListener() anywhere else, so that seems reasonable.
 

- I think the changes to LoggingConfigurer are a good example of how the approach is a little too invasive. To me, ListenerManager and LoggingConfigurer are services, and it's always a smell (to me) when a service is passed into another service via the service interface methods. Why should I need a ListenerManager to configure logging? It feels like ListenerManager is leaking into places it doesn't really belong.

Some alternative approaches:

1. Inject the ListenerManager into the DefaultLoggingConfigurer, and leave the addNnnListener() methods on LoggingConfigurer. DefaultLoggingConfigurer uses the ListenerManager to create 2 broadcasters with type StandardOutputListener. That is, it creates 2 event streams. One stream is for standard out, the other for standard error.

2. Move the above logic out of DefaultLoggingConfigurer, and inject the 2 broadcasters into DefaultLoggingConfigurer. Remove the addNnnListener() methods. This way, DefaultLoggingConfigurer know nothing of ListenerManagers. It just knows it needs to emit std out and std err streams.

interface LoggingConfigurer {
    void configure(LogLevel level)
}

class DefaultLoggingConfigurer {
    public DefaultLoggingConfigurer(StandardOutputListener stdoutBroadcaster, StandardOutputListener stderrBroadcaster) {
       ...
   }

   public void configure(LogLevel level) {
      ... attaches the broadcasters to the appropriate appenders ...
   }
}

I think I like option 2.

I agree, I think option 2 is better.  I didn't think enough about that as I was doing it, and it did feel somewhat wrong to me.  Consider it done.
 

- DefaultInternalRepository should not register itself. In general, I think it's better for a service to simply declare that it is a listener of some type and have the environment take care of the registering, rather than for the service to know how to register itself. This is just a variation of inversion of control.

I disagree.  DefaultInternalRepository requires that it be registered as a listener for it to correctly function.  Exposing it's implementation details to the environment seems wrong.  I would rather it know how to register itself.  I don't see this as an inversion of control issue as the repository class is not finding the listener manager with which it should register, that would be an inversion of control issue.  It's being given the listener manager.  Perhaps you would rather that is was given a ListenerBroadcaster object with which it can register?  That way it still makes clear that it must be registered as a listener in order to function correctly.

- Remote approach looks good, but I would hold off going too far down that path until the native test changes are merged in, because it does pretty much the same stuff.

I didn't know the native test changes did this, that's cool.  I went down this road because we need this functionality to implement the TeamCity plugin stuff, so I did it.  I'll be curious to see what Tom has done here as my current implementation seems very light weight.

Adam

--
John Murph
Automated Logic Research Team

[hdockter]

On Sep 8, 2009, at 9:43 PM, John Murph wrote:

<snip>

>
> - Remote approach looks good, but I would hold off going too far  
> down that path until the native test changes are merged in, because  
> it does pretty much the same stuff.
>
> I didn't know the native test changes did this, that's cool.  I went  
> down this road because we need this functionality to implement the  
> TeamCity plugin stuff, so I did it.  I'll be curious to see what Tom  
> has done here as my current implementation seems very light weight.

I just would like to point out that Tom's stuff might not be part of  
0.8 as it is not production ready yet. It will definitely make it into  
0.9. So it is good to have something for 0.8. I really would like to  
improve the test execution reporting for 0.8. As a default progress  
dots for successfully executing tests and naming a test that fails.

- Hans

--
Hans Dockter
Gradle Project Manager
http://www.gradle.org

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

[John Murph]

I have my latest iteration up on GitHub (at git@...:jmurph/gradle.git branch listener_manager).  I (think I) did everything you requested except

1) I left the StandardOutputListener and StandardErrorListener stuff combined into a single StandardOutputListener because it just seems like those are really one concept and having a single listener made more sense to me.

2) I left DefaultInternalRepository taking a ListenerManager in it's constructor.  I think this is better, but if you still disagree it's two seconds to change it back.

Other than that, I went ahead and changed things.  If I missed anything, just let me know.  If not, you can commit it or let me know and I can get Steve to do so.  (Sorry if I'm being ornery about these two issues, but as the scorpion said to the frog "it's in my nature...")


P.S. Should I start using the "pull request" feature of GitHub to notify you of these kinds of things?  That would help keep me from polluting the gradle-dev mailing list, but then I'm not sure who I would notify, "gradle", "Adam" or "Hans"...

--
John Murph
Automated Logic Research Team

[John Murph]

Adam,

Maybe it's not ready for committing.  When I run "gradlew integTest" I'm getting three errors.  BuildScriptExecutionIntegrationTest, InitScriptExecutionIntegrationTest and SettingsScriptExecutionIntegrationTest are all failing with the same basic error:

Expected: a string containing "quiet message" got: ""

junit.framework.AssertionFailedError:
Expected: a string containing "quiet message"
got: ""

Expected: a string containing "quiet message"
got: ""

at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCallStatic(CallSiteArray.java:48)
at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callStatic(AbstractCallSite.java:167)
at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callStatic(AbstractCallSite.java:179)
at org.gradle.integtests.BuildScriptExecutionIntegrationTest.executesBuildScriptWithCorrectEnvironment(BuildScriptExecutionIntegrationTest.groovy:22)

The problem is that when running from inside IDEA these tests pass.  And I don't see why they wouldn't.  Please see if they are passing for you.  If not, do you see what I'm missing?


--
John Murph
Automated Logic Research Team

[Adam Murdoch-2]

John Murph wrote:
> I have my latest iteration up on GitHub (at
> git@...:jmurph/gradle.git branch listener_manager).  I (think
> I) did everything you requested except
>
> 1) I left the StandardOutputListener and StandardErrorListener stuff
> combined into a single StandardOutputListener because it just seems
> like those are really one concept and having a single listener made
> more sense to me.
>

There's actually 2 concepts here:
- The type of event being received. In this case, we're receiving log
message events.
- The source of the events. In this case, there's 2 sources, namely
stdout log messages and stderr log messages.

The problem with using a single interface for these 2 concepts is that
you statically bind the concepts together. This forces every listener
implementation to deal with the source, even if when doesn't care. For
example, a logging listener which just appends all logging messages to a
text panel must deal with both stdout and stderr events, even though it
does not care about the distinction. This makes it more difficult to
write reusable, composable listener implementations that you can wire
together (dynamically) in different ways.

Another problem with static sources in the listener interface is that
you can't change the set of sources without changing the interface. Or
introducing another, very similar, interface. The set of sources we care
about are quite likely to change. For example, we might want to add some
way to receive all the logging events generated by Ant, regardless of
whether they end up on stdout or stderr. Or perhaps all logging events
generated at info log level. Or perhaps the stderr logging events
generated by the build script.

As a general pattern, I think we should provide small listener
interfaces with maybe 1 method, or possibly 2 methods for a
started/completed type event stream. The interfaces should not encode
the source in the names of the methods (as a parameter of the method is
fine). A listener can pick and choose which of these interfaces it wants
to implement. This makes us resilient to change, and allows listeners to
be composed from reusable pieces.

> 2) I left DefaultInternalRepository taking a ListenerManager in it's
> constructor.  I think this is better, but if you still disagree it's
> two seconds to change it back.
>

I don't feel too strongly about this. It's fine how it is.

> Other than that, I went ahead and changed things.  If I missed
> anything, just let me know.  If not, you can commit it or let me know
> and I can get Steve to do so.  (Sorry if I'm being ornery about these
> two issues, but as the scorpion said to the frog "it's in my nature...")
>
>
> P.S. Should I start using the "pull request" feature of GitHub to
> notify you of these kinds of things?  That would help keep me from
> polluting the gradle-dev mailing list, but then I'm not sure who I
> would notify, "gradle", "Adam" or "Hans"...
>

Personally, I think the mailing list is the best place, but I don't
really mind how we do this.


Adam


---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

[John Murph]

On Mon, Sep 14, 2009 at 2:11 AM, Adam Murdoch <a@...> wrote:

John Murph wrote:

I have my latest iteration up on GitHub (at git@...:jmurph/gradle.git branch listener_manager).  I (think I) did everything you requested except

1) I left the StandardOutputListener and StandardErrorListener stuff combined into a single StandardOutputListener because it just seems like those are really one concept and having a single listener made more sense to me.

There's actually 2 concepts here:
- The type of event being received. In this case, we're receiving log message events.
- The source of the events. In this case, there's 2 sources, namely stdout log messages and stderr log messages.

The problem with using a single interface for these 2 concepts is that you statically bind the concepts together. This forces every listener implementation to deal with the source, even if when doesn't care. For example, a logging listener which just appends all logging messages to a text panel must deal with both stdout and stderr events, even though it does not care about the distinction. This makes it more difficult to write reusable, composable listener implementations that you can wire together (dynamically) in different ways.

Another problem with static sources in the listener interface is that you can't change the set of sources without changing the interface. Or introducing another, very similar, interface. The set of sources we care about are quite likely to change. For example, we might want to add some way to receive all the logging events generated by Ant, regardless of whether they end up on stdout or stderr. Or perhaps all logging events generated at info log level. Or perhaps the stderr logging events generated by the build script.

As a general pattern, I think we should provide small listener interfaces with maybe 1 method, or possibly 2 methods for a started/completed type event stream. The interfaces should not encode the source in the names of the methods (as a parameter of the method is fine). A listener can pick and choose which of these interfaces it wants to implement. This makes us resilient to change, and allows listeners to be composed from reusable pieces.

I think that's generally a good rule, but rules have exceptions for a reason, and I think this is one.  If stdout and stderr were really different sources, I would agree with you.  Since they are two sides of the same coin, and always paired together your argument seems overly academic.  But really, I would change it except I'm not sure how.

The design of ListenerManager is that listeners are defined by their type, not by usage.  That's because in most cases the two are the same, and I didn't like the redundancy.  So, I have a couple of options for splitting these streams back apart but I don't like any of them.

1) Rename the original StandardOutputListener to StandardStreamListener.  Make two subtypes StandardOutputListener and StandardErrorListener that extend StandardStreamListener by are empty.  These would be marker interfaces just to allow ListenerManager and friends to talk about type instead of usage.

2) Change the original StandardOutputListener's onOutput method to take a source parameter (of type String, or some stronger type?).  This would allow one interface to be used for multiple purposes, but causes listeners to get feedback for all sources, even ones they might now know about (such as Ant events).  This is probably the smallest change.

3) Change ListenerManager to be are of usage.  So, a listener would be registered along with a (optional?) usage identifier.  When a broadcaster is requested, the type of the listener and it's intended usage would have to be given.  This complicates ListenerManager and it's usage but supports these kinds of situations in the future without requiring that 1) or 2) be done again for each case.

My vote would obviously be to leave the two streams combined.  What about you (you here means more than just Adam, if anyone else cares I would love to hear your opinions)?


--
John Murph
Automated Logic Research Team

[Adam Murdoch-2]

John Murph wrote:

On Mon, Sep 14, 2009 at 2:11 AM, Adam Murdoch <a@...> wrote:

John Murph wrote:

I have my latest iteration up on GitHub (at git@... branch listener_manager).  I (think I) did everything you requested except

1) I left the StandardOutputListener and StandardErrorListener stuff combined into a single StandardOutputListener because it just seems like those are really one concept and having a single listener made more sense to me.

There's actually 2 concepts here:
- The type of event being received. In this case, we're receiving log message events.
- The source of the events. In this case, there's 2 sources, namely stdout log messages and stderr log messages.

The problem with using a single interface for these 2 concepts is that you statically bind the concepts together. This forces every listener implementation to deal with the source, even if when doesn't care. For example, a logging listener which just appends all logging messages to a text panel must deal with both stdout and stderr events, even though it does not care about the distinction. This makes it more difficult to write reusable, composable listener implementations that you can wire together (dynamically) in different ways.

Another problem with static sources in the listener interface is that you can't change the set of sources without changing the interface. Or introducing another, very similar, interface. The set of sources we care about are quite likely to change. For example, we might want to add some way to receive all the logging events generated by Ant, regardless of whether they end up on stdout or stderr. Or perhaps all logging events generated at info log level. Or perhaps the stderr logging events generated by the build script.

As a general pattern, I think we should provide small listener interfaces with maybe 1 method, or possibly 2 methods for a started/completed type event stream. The interfaces should not encode the source in the names of the methods (as a parameter of the method is fine). A listener can pick and choose which of these interfaces it wants to implement. This makes us resilient to change, and allows listeners to be composed from reusable pieces.

I think that's generally a good rule, but rules have exceptions for a reason, and I think this is one.

We didn't need the exception before we started adding the ListenerManager. We started from having the logging event API follow the rule. ListenerManager is a piece of infrastructure, and shouldn't really have such an impact on the public API. The fact that it has suggests to me that we've not got the infrastructure quite right.

  If stdout and stderr were really different sources, I would agree with you.  Since they are two sides of the same coin, and always paired together your argument seems overly academic.

But they're not always paired together from the consumer's point of view (that is, the user of these APIs).

Sometimes I want to do exactly the same thing for all log messages regardless of where they come from. For example, I might want to append each log message to the end of a text pane in a UI. In this case, I don't care about the distinction between stdout and stderr. I don't want the listener interface to make me do something for stdout and something for stderr.

Sometimes I want to do something with the stderr messages only. For example, I might gather the error log messages up into an email to send at the end of the build. In this case, I don't care about stdout. I don't want the listener interface to make me do something for stdout and something for stderr.

In fact, you can reduce all usages of logging events to combinations of doing the same stuff for all log messages, and doing stuff for just one source. And neither of these cases are well served by a listener interface that has a method for stdout and a method for stderr.

We didn't have these problems with a single method listener interface.

But really, I would change it except I'm not sure how.

The design of ListenerManager is that listeners are defined by their type, not by usage.  That's because in most cases the two are the same, and I didn't like the redundancy.  So, I have a couple of options for splitting these streams back apart but I don't like any of them.

1) Rename the original StandardOutputListener to StandardStreamListener.  Make two subtypes StandardOutputListener and StandardErrorListener that extend StandardStreamListener by are empty.  These would be marker interfaces just to allow ListenerManager and friends to talk about type instead of usage.

2) Change the original StandardOutputListener's onOutput method to take a source parameter (of type String, or some stronger type?).  This would allow one interface to be used for multiple purposes, but causes listeners to get feedback for all sources, even ones they might now know about (such as Ant events).  This is probably the smallest change.

3) Change ListenerManager to be are of usage.  So, a listener would be registered along with a (optional?) usage identifier.  When a broadcaster is requested, the type of the listener and it's intended usage would have to be given.  This complicates ListenerManager and it's usage but supports these kinds of situations in the future without requiring that 1) or 2) be done again for each case.

I suggest we:

- Add methods to the ListenerManager interface so that clients can create anonymous ListenerBroadcaster instances of a given type. Each would be equivalent to a source, but its really up to the client as to what the meaning is.

- Change the implementation of this to also create a global ListenerBroadcaster of the requested type, which receives all events from all anonymous ListenerBroadcaster instances of the same type.

- Inject a ListenerManager into DefaultLoggingConfigurer. It will use the ListenerManager to create 2 anonymous ListenerBroadcaster instances, one for stdout and one for stderr. We keep the LoggingConfigurer.addNnnListener() methods.

- GradleLauncher.addStdNnnListener() delegates to the LoggingConfigurer, which attaches the listener to the appropriate ListenerBroadcaster.

- GradleLauncher.addListener(someStdOutListener) delegates to the ListenerManager, which attaches the listener to the global ListenerBroadcaster.


Adam

[John Murph]

Adam,

Changes made.  See git://github.com/jmurph/gradle.git branch listener_manager.  BuildScriptExecutionIntegrationTest, InitScriptExecutionIntegrationTest and SettingsScriptExecutionIntegrationTest are still failing when I run integTest (even with a clean build), but not from within IDEA.  I'm sure I broke something, but I still have no idea what (esp. since I back out most of the changes to combine stdout and stderr into one listener).  Please try it and commit if you don't see the failure (and you are good with the changes).  Otherwise, next week I'll see if someone here can reproduce the integration test failures in IDEA.

--
John Murph
Automated Logic Research Team

[John Murph]

Adam,

Yay!  I found the integration test problem and have an up-to-date version of the patch on my GitHub account under branch listener_manager.  Please take a look and commit it if you have time, or I can get Steve to commit it tomorrow.


--
John Murph
Automated Logic Research Team

[Adam Murdoch-2]

John Murph wrote:
> Adam,
>
> Yay!  I found the integration test problem and have an up-to-date
> version of the patch on my GitHub account under branch
> listener_manager.  Please take a look and commit it if you have time,
> or I can get Steve to commit it tomorrow.
>

Looks good to me.


Adam


---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email