You are viewing a plain text version of this content. The canonical link for it is here.
Posted to log4j-dev@logging.apache.org by Paul Smith <ps...@aconex.com> on 2007/11/06 01:55:09 UTC
[PROPOSAL] SocketHubAppender change - allow auto port choice
I'd like to propose a change to SocketHubAppender code to allow it
automatically choose a free port on the local host if the Port
property is configured with 0.
This will allow the Zeroconf module to be more useful, and allow
simpler configuration for multiple applications on the same host. We
have an open feature request (see [1]) to allow this, and the only
simple way I can get this to work is to change SocketHubAppender (see
attached patch).
In summary, I've moved the ServerSocket initialisation from the
ServerMonitor.run() method to the ServerMonitor constructor, and when
Port=0, check what port is actually configured, and modify the parent
SocketHubAppender instance port variable so that getPort() exposes the
real port in use.
Sub-classes can then inspect that property after the call to
super.activateOptions(). This allows ZeroconfSocketHubAppender to
broadcast the correct port in use (it would have broadcast 0 without
this change; not very useful).
This doesn't appear to break any of the log4j unit tests, and I've
added unit tests to the Zeroconf module (see second patch) that
confirm behaviour. I've also made the Zeroconf module aware of the
fact that it might not be using an appropriate log4j version, given
that this feature would/might only be present in log4j 1.2.16. Prior
versions of log4j will work fine still, but the automatic port
configuration won't work (it throws an UnsupportedOperationException
if it detects this).
cheers,
Paul
[1] http://issues.apache.org/bugzilla/show_bug.cgi?id=43295
Re: log4j breaking maven builds
Posted by Curt Arnold <ca...@apache.org>.
On Nov 15, 2007, at 2:02 PM, Scott Deboy wrote:
> Can we get the maven dependencies corrected so we don't break maven
> and
> others?
>
> See
> http://www.1060.org/blogxter/entry?
> publicid=9FDCA6751DE11295AD0049F5DB17
> F461&token=
>
Its fixed in the subversion repo. All that would be necessary is to
do a 1.2.16 release. There isn't a good mechanism for changing the
metadata after a release.
There was a suggestion to set the scope to "provided", but from my
reading that isn't appropriate since if you are running or building
JMX or JMS on a J2SE environment those aren't provided by the JRE,
though they would be if you were running on a J2EE. Guess we could
ask on the maven-user list for a definitive suggestion, but I think
optional is appropriate and provided is wrong.
We aren't breaking the building of Maven or Ivy itself as far as I
know. Only that projects built with Maven or Ivy that upgrade to
1.2.15, it should be fixed, but it should not break anyone without a
first action on their part.
So I guess the right action is to proceed with a 1.2.16 release. If
you have any issues that you think must be in 1.2.16, add them as
blockers to bug 43313.
---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
For additional commands, e-mail: log4j-dev-help@logging.apache.org
log4j breaking maven builds
Posted by Scott Deboy <sd...@comotivsystems.com>.
Can we get the maven dependencies corrected so we don't break maven and
others?
See
http://www.1060.org/blogxter/entry?publicid=9FDCA6751DE11295AD0049F5DB17
F461&token=
Scott Deboy
Principal Engineer
COMOTIV SYSTEMS
111 SW Columbia Street Ste. 950
Portland, OR 97201
Office: 503.224.7496
Direct Line: 503.821.6482
Cell: 503.997.1367
Fax: 503.222.0185
sdeboy@comotivsystems.com
www.comotivsystems.com
-----Original Message-----
From: Curt Arnold [mailto:carnold@apache.org]
Sent: Thursday, November 15, 2007 11:19 AM
To: Log4J Developers List
Subject: Re: [PROPOSAL] SocketHubAppender change - allow auto port
choice
I've logged a bug report (43874) for the SocketHubAppender
enhancement and committed the createServerSocket() patch and updated
the changes.xml for 1.2.16. I'm not saying that has to be the final
approach, but I think there is general agreement that we can do
something minimal and safe in 1.2.16 that would allow zeroconf to
work in this case and so moving the discussion to a bug report and
using the SVN to iterate is probably a better process.
---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
For additional commands, e-mail: log4j-dev-help@logging.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
For additional commands, e-mail: log4j-dev-help@logging.apache.org
Re: [PROPOSAL] SocketHubAppender change - allow auto port choice
Posted by Curt Arnold <ca...@apache.org>.
I've logged a bug report (43874) for the SocketHubAppender
enhancement and committed the createServerSocket() patch and updated
the changes.xml for 1.2.16. I'm not saying that has to be the final
approach, but I think there is general agreement that we can do
something minimal and safe in 1.2.16 that would allow zeroconf to
work in this case and so moving the discussion to a bug report and
using the SVN to iterate is probably a better process.
---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
For additional commands, e-mail: log4j-dev-help@logging.apache.org
Re: [PROPOSAL] SocketHubAppender change - allow auto port choice
Posted by Curt Arnold <ca...@apache.org>.
On Nov 15, 2007, at 3:35 PM, Paul Smith wrote:
> 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
>
I'm sure the rationale behind the original design of
SocketHubAppender is probably not in the archives, so it is hard to
tell if there was some significant motivation for having the
ServerSocket constructor on a distinct thread or if it was just
simpler to code. However, it seems undesirable to change the
behavior of SocketHubAppender when there is no known benefit to a
user of SocketHubAppender and some risk, likely slight, of losing
that desired behavior.
It doesn't seem that difficult to adapt to the changes in
ZeroConfSocketHubAppender. Since ZeroConfSocketHubAppender has never
been formally released and SocketHubAppender has been in log4j for a
long time, I'm much more inclined to make major or moderate changes
in ZeroConfSocketHubAppender than making even minor changes in
SocketHubApppender.
It shouldn't need a tricky wait mechanism, you just register in
your override of createServerSocket(). Of course, if you want
ZeroConfSocketHubAppender to work with earlier log4j's, then you'd
need to sniff either the log4j version of the presence of
SocketHubAppender.createSocketServer, but that would be a lot less
code than the log4j 1.3 sniffing already in the code.
I've taken a pass at cleaning up the old log4j 1.3 reflection code
that was calling the internal Logger.getLogger() call and replaced it
with equivalent LogLog.debug or LogLog.error code. Haven't tested
it, but I don't see any reason it wouldn't work (or couldn't be made
to work) without changing the behavior of SocketHubAppender.
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version
2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.apache.log4j.net;
import org.apache.log4j.helpers.LogLog;
import javax.jmdns.JmDNS;
import javax.jmdns.ServiceInfo;
import java.io.IOException;
import java.net.ServerSocket;
/**
* A sub-class of SocketHubAppender that broadcasts its configuration
via Zeroconf.
*
* This allows Zeroconf aware applications such as Chainsaw to be able
to detect them, and automatically configure
* themselves to be able to connect to them.
*
* @author psmith
*
*/
public class ZeroConfSocketHubAppender extends SocketHubAppender {
public static final String
DEFAULT_ZEROCONF_ZONE="_log4j._tcp.local.";
private String zeroConfZone = DEFAULT_ZEROCONF_ZONE;
/**
* True if running on log4j 1.2.16 or later.
*/
private boolean hasCreateServerSocket;
private ServiceInfo serviceInfo;
public ZeroConfSocketHubAppender() {
setName("SocketHubAppender");
try {
SocketHubAppender.class.getMethod("createServerSocket",
new Class[] { int.class});
hasCreateServerSocket = true;
} catch(NoSuchMethodException e) {
hasCreateServerSocket = false;
}
}
public void activateOptions() {
super.activateOptions();
if (!hasCreateServerSocket) {
if (getPort() == 0) {
LogLog.warn("Unable to register
ZeroConfSocketHubAppender with unspecified port using log4j versions
prior to log4j 1.2.16");
} else {
registerService(getPort());
}
}
}
protected ServerSocket createServerSocket(final int port) throws
IOException {
ServerSocket socket = new ServerSocket(port);
registerService(socket.getLocalPort());
return socket;
}
private void registerService(int port) {
try {
JmDNS jmDNS = Zeroconf4log4j.getInstance();
serviceInfo = new ServiceInfo(zeroConfZone, getName(),
port, "SocketHubAppender on port " + port);
LogLog.debug("Registering this SocketHubAppender as :" +
serviceInfo);
jmDNS.registerService(serviceInfo);
} catch (IOException e) {
LogLog.error("Failed to instantiate JmDNS to broadcast
via ZeroConf, will now operate in simple SocketHubAppender mode");
}
}
/**
* Returns the ZeroConf domain that will be used to register
this 'device'.
*
* @return String ZeroConf zone
*/
public String getZeroConfZone() {
return zeroConfZone;
}
/**
* Sets the ZeroConf zone to register this device under, BE
CAREFUL with this value
* as ZeroConf has some weird naming conventions, it should
start with an "_" and end in a ".",
* if you're not sure about this value might I suggest that you
leave it at the default value
* which is specified in {@link #DEFAULT_ZEROCONF_ZONE }.
*
* This method does NO(0, zero, pun not intended) checks on this
value.
*
* @param zeroConfZone
*/
public void setZeroConfZone(String zeroConfZone) {
// TODO work out a sane checking mechanism that verifies the
value is a correct ZeroConf zone
this.zeroConfZone = zeroConfZone;
}
public synchronized void close() {
super.close();
JmDNS jmDNS = Zeroconf4log4j.getInstance();
if (serviceInfo != null) {
try {
LogLog.debug("Deregistering this SocketHubAppender
(" + serviceInfo + ")");
jmDNS.unregisterService(serviceInfo);
} catch (Exception e) {
LogLog.error("Failed to instantiate JmDNS to
broadcast via ZeroConf, will now operate in simple SocketHubAppender
mode");
}
}
}
}
---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
For additional commands, e-mail: log4j-dev-help@logging.apache.org
Re: [PROPOSAL] SocketHubAppender change - allow auto port choice
Posted by Paul Smith <ps...@aconex.com>.
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.
Re: [PROPOSAL] SocketHubAppender change - allow auto port choice
Posted by Paul Smith <ps...@aconex.com>.
>>
>
> That chunk seems just like the IDE at work moving things around.
>
Honestly, is that really that much of a deal? I agree a total
reformat of the class is not a good idea, but I don't see the need to
get too anal about something like that..
>>
>
> 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.
>
Looking at the ServerSocket I didn't really see anything much of use
part from the address and port, but I guess it's just as easy to pass
the whole thing.
Thanks Curt for committing a change.
Paul
---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
For additional commands, e-mail: log4j-dev-help@logging.apache.org
Re: [PROPOSAL] SocketHubAppender change - allow auto port choice
Posted by Curt Arnold <ca...@apache.org>.
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
Re: [PROPOSAL] SocketHubAppender change - allow auto port choice
Posted by Paul Smith <ps...@aconex.com>.
On 07/11/2007, at 7:15 PM, Curt Arnold wrote:
>
> On Nov 7, 2007, at 1:55 AM, Curt Arnold wrote:
>
>> Okay, I'm about ready to fall over, but I looked at zeroconf and
>> see your motivation for moving the binding onto the main thread so
>> you continue the set up in
>> ZeroConfSocketHubAppender.activateOptions. However instead of
>> changing the behavior, I'd be more inclined to make a few changes
>> to SocketHubAppender to make it more amenable to extension but keep
>> the old behavior. I'd suggest:
>>
>> Moving ServerMonitor out to a separate public class.
>> Move the self-start in ServerMonitor to
>> SocketHubAppender.startServer.
>> Make SocketHubAppender.startServer protected and overload it in
>> ZeroConf.
>> Add a protected SocketHubAppender.stopServer and move the
>> ServerMonitor.stopMonitor code there.
>>
>
> I'd suggest that you not take code suggestions from me too seriously
> this late at night. There is probably a cleaner way to be able to
> provide a hook for the zeroconf registration code to run. Maybe
> adding like adding
>
> protected void socketBound(ServerSocket s) {
> }
>
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,
@@ -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. */
@@ -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) {
---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
For additional commands, e-mail: log4j-dev-help@logging.apache.org
Re: [PROPOSAL] SocketHubAppender change - allow auto port choice
Posted by Paul Smith <ps...@aconex.com>.
On 07/11/2007, at 7:15 PM, Curt Arnold wrote:
>
> On Nov 7, 2007, at 1:55 AM, Curt Arnold wrote:
>
>> Okay, I'm about ready to fall over, but I looked at zeroconf and
>> see your motivation for moving the binding onto the main thread so
>> you continue the set up in
>> ZeroConfSocketHubAppender.activateOptions. However instead of
>> changing the behavior, I'd be more inclined to make a few changes
>> to SocketHubAppender to make it more amenable to extension but keep
>> the old behavior. I'd suggest:
>>
>> Moving ServerMonitor out to a separate public class.
>> Move the self-start in ServerMonitor to
>> SocketHubAppender.startServer.
>> Make SocketHubAppender.startServer protected and overload it in
>> ZeroConf.
>> Add a protected SocketHubAppender.stopServer and move the
>> ServerMonitor.stopMonitor code there.
>>
>
> I'd suggest that you not take code suggestions from me too seriously
> this late at night. There is probably a cleaner way to be able to
> provide a hook for the zeroconf registration code to run. Maybe
> adding like adding
>
> protected void socketBound(ServerSocket s) {
> }
>
> notification method to SocketHubAppender and then hook it at
> ZeroConfSocketHubAppender to publish the address.
Before I forget (currently in the middle of quartely software release
week.. extremely overloaded, I'm not ignoring this..), yeah I agree
with this approach. I'd be tempted as part of the separation to
introduce a standard Java Listener interface and have socketBound and
socketAccept callbacks while where there.
I'm not exactly sure when I'll get to it though, this week is going to
be busy. May not be till Friday.
cheers,
Paul
---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
For additional commands, e-mail: log4j-dev-help@logging.apache.org
Re: [PROPOSAL] SocketHubAppender change - allow auto port choice
Posted by Curt Arnold <ca...@apache.org>.
On Nov 7, 2007, at 1:55 AM, Curt Arnold wrote:
> Okay, I'm about ready to fall over, but I looked at zeroconf and
> see your motivation for moving the binding onto the main thread so
> you continue the set up in
> ZeroConfSocketHubAppender.activateOptions. However instead of
> changing the behavior, I'd be more inclined to make a few changes
> to SocketHubAppender to make it more amenable to extension but keep
> the old behavior. I'd suggest:
>
> Moving ServerMonitor out to a separate public class.
> Move the self-start in ServerMonitor to SocketHubAppender.startServer.
> Make SocketHubAppender.startServer protected and overload it in
> ZeroConf.
> Add a protected SocketHubAppender.stopServer and move the
> ServerMonitor.stopMonitor code there.
>
I'd suggest that you not take code suggestions from me too seriously
this late at night. There is probably a cleaner way to be able to
provide a hook for the zeroconf registration code to run. Maybe
adding like adding
protected void socketBound(ServerSocket s) {
}
notification method to SocketHubAppender and then hook it at
ZeroConfSocketHubAppender to publish the address.
---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
For additional commands, e-mail: log4j-dev-help@logging.apache.org
Re: [PROPOSAL] SocketHubAppender change - allow auto port choice
Posted by Curt Arnold <ca...@apache.org>.
On Nov 5, 2007, at 6:55 PM, Paul Smith wrote:
> I'd like to propose a change to SocketHubAppender code to allow it
> automatically choose a free port on the local host if the Port
> property is configured with 0.
>
> This will allow the Zeroconf module to be more useful, and allow
> simpler configuration for multiple applications on the same host.
> We have an open feature request (see [1]) to allow this, and the
> only simple way I can get this to work is to change
> SocketHubAppender (see attached patch).
>
> In summary, I've moved the ServerSocket initialisation from the
> ServerMonitor.run() method to the ServerMonitor constructor, and
> when Port=0, check what port is actually configured, and modify the
> parent SocketHubAppender instance port variable so that getPort()
> exposes the real port in use.
>
> Sub-classes can then inspect that property after the call to
> super.activateOptions(). This allows ZeroconfSocketHubAppender to
> broadcast the correct port in use (it would have broadcast 0
> without this change; not very useful).
>
> This doesn't appear to break any of the log4j unit tests, and I've
> added unit tests to the Zeroconf module (see second patch) that
> confirm behaviour. I've also made the Zeroconf module aware of the
> fact that it might not be using an appropriate log4j version, given
> that this feature would/might only be present in log4j 1.2.16.
> Prior versions of log4j will work fine still, but the automatic
> port configuration won't work (it throws an
> UnsupportedOperationException if it detects this).
>
> cheers,
>
> Paul
>
> [1] http://issues.apache.org/bugzilla/show_bug.cgi?id=43295
>
> <log4j_sockethubappender.patch>
> <log4j_zeroconf.patch>
>
>
I've only briefly looked at the SocketHubAppender patch. A few
comments/questions:
There are a quite a few whitespace only changes that make it
difficult to review only the code that changed. I'm guessing it is
your IDE trying to be helpful, but it would be nice if you could
change the configuration not to do that.
I think I'd be more comfortable to add a distinct accessor for the
bound port instead of overwriting the port specification. If you did
that, you could distinguish between the case where you did not
specify a port and one was assigned versus explicitly specifying that
port. If you did that, I think you could avoid moving the socket
initialization from the run method to the constructor.
Okay, I'm about ready to fall over, but I looked at zeroconf and see
your motivation for moving the binding onto the main thread so you
continue the set up in ZeroConfSocketHubAppender.activateOptions.
However instead of changing the behavior, I'd be more inclined to
make a few changes to SocketHubAppender to make it more amenable to
extension but keep the old behavior. I'd suggest:
Moving ServerMonitor out to a separate public class.
Move the self-start in ServerMonitor to SocketHubAppender.startServer.
Make SocketHubAppender.startServer protected and overload it in
ZeroConf.
Add a protected SocketHubAppender.stopServer and move the
ServerMonitor.stopMonitor code there.
---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
For additional commands, e-mail: log4j-dev-help@logging.apache.org
RE: [PROPOSAL] SocketHubAppender change - allow auto port choice
Posted by Scott Deboy <sd...@comotivsystems.com>.
+1
Scott
-----Original Message-----
From: Paul Smith [mailto:psmith@aconex.com]
Sent: Mon 11/5/2007 4:55 PM
To: Log4J Developers List
Subject: [PROPOSAL] SocketHubAppender change - allow auto port choice
I'd like to propose a change to SocketHubAppender code to allow it
automatically choose a free port on the local host if the Port
property is configured with 0.
This will allow the Zeroconf module to be more useful, and allow
simpler configuration for multiple applications on the same host. We
have an open feature request (see [1]) to allow this, and the only
simple way I can get this to work is to change SocketHubAppender (see
attached patch).
In summary, I've moved the ServerSocket initialisation from the
ServerMonitor.run() method to the ServerMonitor constructor, and when
Port=0, check what port is actually configured, and modify the parent
SocketHubAppender instance port variable so that getPort() exposes the
real port in use.
Sub-classes can then inspect that property after the call to
super.activateOptions(). This allows ZeroconfSocketHubAppender to
broadcast the correct port in use (it would have broadcast 0 without
this change; not very useful).
This doesn't appear to break any of the log4j unit tests, and I've
added unit tests to the Zeroconf module (see second patch) that
confirm behaviour. I've also made the Zeroconf module aware of the
fact that it might not be using an appropriate log4j version, given
that this feature would/might only be present in log4j 1.2.16. Prior
versions of log4j will work fine still, but the automatic port
configuration won't work (it throws an UnsupportedOperationException
if it detects this).
cheers,
Paul
[1] http://issues.apache.org/bugzilla/show_bug.cgi?id=43295