logging-log4j-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Paul Smith <psm...@aconex.com>
Subject Re: [PROPOSAL] SocketHubAppender change - allow auto port choice
Date Thu, 15 Nov 2007 21:35:13 GMT
actually I've found a problem with the commit.

If ZeroconfSocketHubAppender class calls super.activateOptions() which  
then creates the SocketServer, the actual creation of the ServerSocket  
is done by another thread (see the SocketMonitor constructor), which  
means the call back to the protected method to create the socket is  
done via another thread.

This means that after super.activateOptions() call returns,  
ZeroconfSocketHubAppender still doesn't know what the port is going to  
be used (it's a timing thing).

I think the call to create the ServerSocket needs to be done in the  
SocketMonitor constructor before the thread is started as I had  
originally in my previous patch.  Otherwise subclasses have to  
implement a tricky wait mechanism to see what port is used before  
fully activating themselves.

Can we please move that into the constructor?

Paul


On 16/11/2007, at 5:37 AM, Curt Arnold wrote:

>
> On Nov 15, 2007, at 4:36 AM, Paul Smith wrote:
>
>>
>> Ok, i'm a bit tired, so I ended up just going simple and pretty  
>> much following Curt's idea, see below.  Thoughts?
>>
>> Index: src/main/java/org/apache/log4j/net/SocketHubAppender.java
>> ===================================================================
>> --- src/main/java/org/apache/log4j/net/SocketHubAppender.java	 
>> (revision 592202)
>> +++ src/main/java/org/apache/log4j/net/SocketHubAppender.java	 
>> (working copy)
>> @@ -17,18 +17,18 @@
>>
>> package org.apache.log4j.net;
>>
>> -import java.util.Vector;
>> -import java.net.Socket;
>> -import java.net.ServerSocket;
>> -import java.net.SocketException;
>> -import java.io.ObjectOutputStream;
>> import java.io.IOException;
>> import java.io.InterruptedIOException;
>> +import java.io.ObjectOutputStream;
>> import java.net.InetAddress;
>> +import java.net.ServerSocket;
>> +import java.net.Socket;
>> +import java.net.SocketException;
>> +import java.util.Vector;
>>
>> +import org.apache.log4j.AppenderSkeleton;
>> import org.apache.log4j.helpers.LogLog;
>> import org.apache.log4j.spi.LoggingEvent;
>> -import org.apache.log4j.AppenderSkeleton;
>>
>> /**
>>   Sends {@link LoggingEvent} objects to a set of remote log servers,
>>
>
> That chunk seems just like the IDE at work moving things around.
>
>> @@ -271,6 +271,19 @@
>>   }
>>
>>   /**
>> +   * Once the {@link ServerSocket} is created and bound, this  
>> method is called to notify the class the details
>> +   * of the address and port chosen.  In the standard  
>> SocketHubAppender case this will be the
>> +   * requested port, but if the port is specified as 0, then a  
>> random port is chosen by the underlying OS.
>> +   * Child classes may be interested in the actual port chosen,  
>> and this method may be overridden
>> +   * and used to gain the details of the actual socket being  
>> listened on.
>> +   * @param address
>> +   * @param actualPortUsed
>> +   */
>> +  protected void socketBound(InetAddress address, int  
>> actualPortUsed) {
>> +
>> +  }
>> +
>> +  /**
>>     This class is used internally to monitor a ServerSocket
>>     and register new connections in a vector passed in the
>>     constructor. */
>
> Will discuss later in message.
>
>
>> @@ -280,6 +293,7 @@
>>     private Vector oosList;
>>     private boolean keepRunning;
>>     private Thread monitorThread;
>> +    private ServerSocket serverSocket;
>>
>>     /**
>>       Create a thread and start the monitor. */
>> @@ -288,9 +302,15 @@
>>       port = _port;
>>       oosList = _oosList;
>>       keepRunning = true;
>> +      try {
>> +        serverSocket = new ServerSocket(port);
>> +      } catch (IOException e) {
>> +        throw new RuntimeException("Failed to create a  
>> ServerSocket", e);
>> +      }
>>       monitorThread = new Thread(this);
>>       monitorThread.setDaemon(true);
>>       monitorThread.start();
>> +      socketBound(serverSocket.getInetAddress(),  
>> serverSocket.getLocalPort());
>>     }
>>
>>     /**
>> @@ -320,9 +340,8 @@
>>       they connect to the socket. */
>>     public
>>     void run() {
>> -      ServerSocket serverSocket = null;
>> +
>>       try {
>> -        serverSocket = new ServerSocket(port);
>>         serverSocket.setSoTimeout(1000);
>>       }
>>       catch (Exception e) {
>>
>>
>
> This still moves behavior around a bit.  I noticed that the inner  
> class is not defined as static, so it has access to all the methods  
> and members of its enclosing class, so you could keep the same  
> notification method, but minimize the changes to the SocketMonitor  
> by just calling socketBound after the existing socket creation code.
>
> @@ -323,6 +336,7 @@
>       ServerSocket serverSocket = null;
>       try {
>         serverSocket = new ServerSocket(port);
> +        socketBound(serverSocket.getInetAddress(),  
> serverSocket.getLocalPort());
>         serverSocket.setSoTimeout(1000);
>       }
>       catch (Exception e) {
>
> But I didn't see much benefit from hiding the rest of the  
> serverSocket from the extending class, so I first simplified the  
> code to:
>
> @@ -323,6 +336,7 @@
>       ServerSocket serverSocket = null;
>       try {
>         serverSocket = new ServerSocket(port);
> +        socketBound(serverSocket);
>         serverSocket.setSoTimeout(1000);
>       }
>       catch (Exception e) {
>
> which lets the extending class extract as much or little info out of  
> the class as possible.  But once I did that, it seemed even more  
> simple to use a protected method to create the socket which is  
> easier to explain and lets the extending class do everything the  
> other approaches do and more.
>
> Index: src/main/java/org/apache/log4j/net/SocketHubAppender.java
> ===================================================================
> --- src/main/java/org/apache/log4j/net/SocketHubAppender.java	 
> (revision 592094)
> +++ src/main/java/org/apache/log4j/net/SocketHubAppender.java	 
> (working copy)
> @@ -271,6 +271,16 @@
>   }
>
>   /**
> +   * Creates a server socket to accept connections.
> +   * @param socketPort port on which the socket should listen, may  
> be zero.
> +   * @return new socket.
> +   * @throws IOException IO error when opening the socket.
> +   */
> +  protected ServerSocket createServerSocket(final int socketPort)  
> throws IOException {
> +      return new ServerSocket(socketPort);
> +  }
> +
> +  /**
>     This class is used internally to monitor a ServerSocket
>     and register new connections in a vector passed in the
>     constructor. */
> @@ -322,7 +332,7 @@
>     void run() {
>       ServerSocket serverSocket = null;
>       try {
> -        serverSocket = new ServerSocket(port);
> +        serverSocket = createServerSocket(port);
>         serverSocket.setSoTimeout(1000);
>       }
>       catch (Exception e) {
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
> For additional commands, e-mail: log4j-dev-help@logging.apache.org
>

Paul Smith
Core Engineering Manager

Aconex
The easy way to save time and money on your project

696 Bourke Street, Melbourne,
VIC 3000, Australia
Tel: +61 3 9240 0200  Fax: +61 3 9240 0299
Email: psmith@aconex.com  www.aconex.com

This email and any attachments are intended solely for the addressee.  
The contents may be privileged, confidential and/or subject to  
copyright or other applicable law. No confidentiality or privilege is  
lost by an erroneous transmission. If you have received this e-mail in  
error, please let us know by reply e-mail and delete or destroy this  
mail and all copies. If you are not the intended recipient of this  
message you must not disseminate, copy or take any action in reliance  
on it. The sender takes no responsibility for the effect of this  
message upon the recipient's computer system.




Mime
View raw message