activemq-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "solprovider (JIRA)" <j...@apache.org>
Subject [jira] Issue Comment Edited: (AMQ-1254) Kaha Store puts a non-string into System properties
Date Sat, 23 Jun 2007 22:02:33 GMT

    [ https://issues.apache.org/activemq/browse/AMQ-1254?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_39495
] 

solprovider edited comment on AMQ-1254 at 6/23/07 3:01 PM:
-----------------------------------------------------------

The reason amq is using this poor design is a programmer did not read the docs for the System
and Properties classes: "Because Properties inherits from Hashtable, the put and putAll methods
can be applied to a Properties object. Their use is strongly discouraged as they allow the
caller to insert entries whose keys or values are not Strings. The setProperty method should
be used instead."  ActiveMQ even contains code that will break if System.Properties contains
elements that cannot be cast to String.

To fix this:
1. Discard function getVmLockSet().  The locks cannot use a Set.
2. Change lock() and unlock() to this code:

{code:title=Good Locking|borderStyle=solid}
private synchronized void lock() throws IOException {
	if (!disableLocking && directory != null && lock == null) {
		String key = getClass().getName() + ".lock." + directory.getCanonicalPath().replaceAll(":/\\\\",
".");
		String property = System.getProperty(key);
		if ((null == property) || ("" == property)) {
			if (!brokenFileLock) {
				lock = rootIndexManager.getLock();
				if (lock == null) {
throw new StoreLockedExcpetion("Kaha Store " + directory.getName() + "  is already opened
by another application");
				} else
					System.setProperty(key, "LOCK"); //Could store datetime of lock.
			}
		} else { //already locked
throw new StoreLockedExcpetion("Kaha Store " + directory.getName() + " is already opened by
this application.");
		}
	}
}
private synchronized void unlock() throws IOException {
	if (!disableLocking && (null != directory) && (null != lock)) {
		String key = getClass().getName() + ".lock." + directory.getCanonicalPath().replaceAll("[:/\\\\]",
".");
		if (null != System.getProperty(key))
			System.setProperty(key, "");
		if (lock.isValid()) {
			lock.release();
		}
		lock = null;
	}
}
{code}

This should be faster since we are not retrieving a Set<String> (but it will slow code
that checks every System.Property.) This should use less memory since we do not have the overhead
of one Set<String>. This does not violate System.Properties by bypassing its functions
to use the parent HashMap functions.

Warning: I am not set up to work with Java 1.5 (required by this class) so this needs to be
tested.


 was:
The reason amq is using this poor design is a programmer did not read the docs for the System
and Properties classes: "Because Properties inherits from Hashtable, the put and putAll methods
can be applied to a Properties object. Their use is strongly discouraged as they allow the
caller to insert entries whose keys or values are not Strings. The setProperty method should
be used instead."  ActiveMQ even contains code that will break if System.Properties contains
elements that cannot be cast to String.

To fix this:
1. Discard function getVmLockSet().  The locks cannot use a Set.
2. Change lock() and unlock() to this code:

{code:title=Good Locking|borderStyle=solid}
private synchronized void lock() throws IOException {
	if (!disableLocking && directory != null && lock == null) {
		String key = getClass().getName() + ".lock." + directory.getCanonicalPath().replaceAll(":/\\\\",
".");
		String property = System.getProperty(key);
		if ((null == property) || ("" == property)) {
			if (!brokenFileLock) {
				lock = rootIndexManager.getLock();
				if (lock == null) {
throw new StoreLockedExcpetion("Kaha Store " + directory.getName() + "  is already opened
by another application");
				} else
					System.setProperty(key, "LOCK"); //Could store datetime of lock.
			}
		} else { //already locked
throw new StoreLockedExcpetion("Kaha Store " + directory.getName() + " is already opened by
this application.");
		}
	}
}
private synchronized void unlock() throws IOException {
	if (!disableLocking && (null != directory) && (null != lock)) {
		String key = getClass().getName() + ".lock." + directory.getCanonicalPath().replaceAll(":/\\\\",
".");
		if (null != System.getProperty(key))
			System.setProperty(key, "");
		if (lock.isValid()) {
			lock.release();
		}
		lock = null;
	}
}
{code}

This should be faster since we are not retrieving a Set<String> (but it will slow code
that checks every System.Property.) This should use less memory since we do not have the overhead
of one Set<String>. This does not violate System.Properties by bypassing its functions
to use the parent HashMap functions.

Warning: I am not set up to work with Java 1.5 (required by this class) so this needs to be
tested.

> Kaha Store puts a non-string into System properties
> ---------------------------------------------------
>
>                 Key: AMQ-1254
>                 URL: https://issues.apache.org/activemq/browse/AMQ-1254
>             Project: ActiveMQ
>          Issue Type: Bug
>    Affects Versions: 4.1.2
>            Reporter: David Jencks
>         Attachments: Kaha-Store.patch
>
>
> KahaStore puts a hashmap into SystemProperties, which causes problems with programs that
expect only strings as properties.  In particular some versions of Hibernate assume all system
properties are strings: this is causing difficulties running roller in geronimo 2.0
> Attached is a proposed solution.  I have no idea how to test it.  I get the same 7 failures
and one error building amq with and without the change.
> The proposal stores the list of locked directories in a string and converts it back and
forth to a map whenever it is accessed.  I use a constant string as the vm-wide lock monitor
formerly provided by the HashSet.  According to the String javadoc constant strings are intern()ed
and the same instance is provided in any classloader: this makes it suitable for a vm-wide
lock monitor.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


Mime
View raw message