Jump to: navigation, search

Difference between revisions of "Virgo/Committers"

m (Final parameters and local variables)
m (Coding page created)
Line 33: Line 33:
 
Virgo code is thread safe unless specifically stated. Achieving thread safety in Java is not easy. We recommend "[http://www.javaconcurrencyinpractice.com/ Java Concurrency in Practice]" for a good grounding in Java concurrency.
 
Virgo code is thread safe unless specifically stated. Achieving thread safety in Java is not easy. We recommend "[http://www.javaconcurrencyinpractice.com/ Java Concurrency in Practice]" for a good grounding in Java concurrency.
  
== Code Style ==
+
[[Virgo/Committers/Coding]]
 
+
All Virgo code '''must''' adhere to the following code style requirements.
+
 
+
=== Code Formatting and Templates ===
+
 
+
Eclipse code templates, formatter, and clean up profile are available in the <b><code lang="java">web server</code></b> git repository in the <b><code lang="java">doc/code-style/eclipse</code></b> directory.
+
 
+
All code committed to the repository must adhere to these templates and must be formatted to match this code style.
+
 
+
==== A note on the New File template ====
+
 
+
In the code templates for Eclipse, there is a copyright and licence header for new Java files.  This will generate a ''partial copyright statement'' which must be updated with the name of the copyright holder before the code is committed.  For example:
+
 
+
<pre>/*
+
* This file is part of the Eclipse Virgo project.
+
*
+
* Copyright (c) 2010 copyright_holder
+
* ...
+
*/</pre>
+
should be updated to, for ''My Company Inc.'':
+
<pre>/*
+
* This file is part of the Eclipse Virgo project.
+
*
+
* Copyright (c) 2010 My Company Inc.
+
* ...
+
*/</pre>
+
before committing.  The easiest way of achieving this is to update the Comment->File template in the Eclipse Java Code templates ''after'' you have imported the templates and ''before'' you create new files.
+
 
+
=== Coding Conventions ===
+
 
+
Virgo code conforms to the [http://java.sun.com/docs/codeconv/html/CodeConvTOC.doc.html Sun Java Coding Conventions] except for the specific deviations from these conventions detailed below.
+
 
+
==== Interface definitions ====
+
 
+
Interface method definitions must '''not''' include any visibility modifier or the 'abstract' modifier. The code below shows an ''incorrect'' interface definition:
+
 
+
<pre>
+
public interface MyInterface {
+
 
+
public abstract void doSomething();
+
}
+
</pre>
+
 
+
The ''correct'' definition is shown below:
+
 
+
<pre>
+
public interface MyInterface {
+
 
+
void doSomething();
+
}
+
</pre>
+
 
+
==== Final parameters and local variables ====
+
 
+
The use of the '''final''' qualifier on method parameters and local variables is strongly discouraged. The reason is that these uses of '''final''' have no actual benefit (or semantic effect) in practice (even as a  hint to the compiler) and that the keyword simply makes the code more verbose -- and therefore harder to read.
+
 
+
Valid scenarios for '''final''' usage are where the keyword ''does'' have semantic effect, including:
+
* passing state to anonymous type instances; and
+
* preservation of an important internal method contract.
+
 
+
Typically, instance variables (state) in instantiated classes will be declared '''final''' ''whenever it is possible to do so'' as this is a significant contributor to ''thread-safe'' code.
+
 
+
(Note that this can lead to large numbers of parameters on constructors of complex classes.  See the '''Builder''' pattern in [http://java.sun.com/docs/books/effective/ Effective Java]  for ways round this.)
+
 
+
== Concurrent Programming Guidelines ==
+
 
+
=== Levels of Concurrent Support ===
+
 
+
A class can either be thread safe or not. A class is thread safe if its invariants are protected when accessed simultaneously by multiple threads.
+
 
+
=== Required Level of Concurrent Support ===
+
 
+
* All classes that form part of the public API of their bundle, or serve as an externally accessible implementation of that public API are '''required''' to be thread safe.
+
 
+
* Public classes that are not intended to be shared by multiple threads may be coded as non thread safe, but this in discouraged since single-threaded usage cannot easily be enforced.
+
 
+
* Classes that are internal to a bundle are free to be non thread safe. However, if these classes are used as part of the implementation of the bundle API then the API classes must guarantee thread safe access to their delegate classes.
+
 
+
=== Alien Calls ===
+
 
+
An alien call is any call to a component outside of the control of the caller. Holding a lock while making an alien call is deadlock-prone and '''forbidden'''. The big question is what constitutes a component. Treating each class as a component from the perspective of alien calls is safe but can be too limiting at times. A package (within a bundle) is a more useful component in practice. In some situations, it may be necessary to code alien calls across packages within a bundle, but extreme care must be taken to avoid deadlocks. Alien calls between bundles are '''forbidden'''.
+
 
+
=== Use of Client Locking Protocols ===
+
 
+
A client locking protocol is a contract between a class and its consumers. This contract outlines how the calling class can cooperate in the locking of the callee class. An example of a client locking protocol in common usage can be found in Hashtable. When performing composite operations on a Hashtable it is possible to synchronize on the Hashtable instance and participate cooperatively in the locking inside the Hashtable.
+
 
+
In Virgo, the use of client locking protocols is '''forbidden''' because they can very easily lead to deadlocks. If a class in Virgo needs to allow composite operations on its internal data structures, it should support these operations in its API.
+
 
+
=== Use of synchronized Methods ===
+
 
+
The use of synchronized methods in Virgo is '''forbidden'''. A class with synchronized methods implicitly exposes its monitor object (itself) to the outside world.
+
 
+
In a multi-threaded environment this can easily lead to deadlocks. If class Foo synchronizes on an instance of Bar, and these instances of Bar synchronize on 'this', then access of Foo and Bar across different threads can easily deadlock.
+
 
+
Note also that explicit 'synchronized(this)' blocks are also '''forbidden'''.
+
 
+
A simple alternative for classes wishing to maintain some internal synchronization is to use an internal monitor field:
+
 
+
<pre>
+
public class MyClass {
+
 
+
private final Object monitor = new Object();
+
+
public void doSomething() {
+
synchronized(this.monitor) {
+
// logic here
+
}
+
}
+
}
+
</pre>
+
 
+
'''Important:''' The monitor field must be declared as final otherwise synchronizing on it is non-deterministic due to Java Memory Model visibility rules.
+
 
+
== Code Visibility ==
+
 
+
=== Types ===
+
 
+
Types should always have the minimum visibility consistent with their use.
+
 
+
=== Methods ===
+
 
+
Methods should have the minimum visibility consistent with their use. Methods that form part of the public contract of a type should of course be public but methods that are used to implement that contract should be private.
+
 
+
When allowing extension, protected methods can be used to provide extension points for subclasses. Protected final methods may be used to provide functionality to subtypes but that cannot be extended.
+
 
+
It is valid to relax the visibility of methods to make unit testing easier. In this case, it is common to change a private method to have default access so that test classes in the same package can access it.
+
 
+
=== Static Fields ===
+
 
+
For immutable static fields, such as those used in constants, any access modifier is valid. For mutable static state, private access '''must''' always be used and mutations to the state should be done via accessor methods.
+
 
+
=== Instance Fields ===
+
 
+
In almost all cases fields should be private. Providing subclasses with access to state ''can'' be done with protected fields but protected accessor methods are preferred.
+
 
+
== Type Naming Conventions ==
+
 
+
The following type naming conventions must be adhered to for both runtime and test classes.
+
 
+
=== Interfaces ===
+
 
+
When implementing an interface the use of the 'Impl' suffix is not permitted.
+
 
+
Ideally choose a name that truly reflects the implementation detail. An example of this is XmlConfigurationSource, an implementation of ConfigurationSource that uses XML to read configuration data.
+
 
+
If no better name is available, the implementation should be named by prefixing Standard to the interface name. For example, the interface Kernel could have an implementation named StandardKernel.
+
 
+
=== Test Cases ===
+
 
+
All test case types must have their names suffixed with 'Tests', for example StandardKernelTests.
+
 
+
== JavaDoc Guidelines ==
+
 
+
=== Useful Resources ===
+
 
+
* [http://java.sun.com/j2se/javadoc/writingdoccomments/ Sun Writing Doc Comments Guide]
+
* [http://java.sun.com/j2se/1.5.0/docs/tooldocs/windows/javadoc.html#javadoctags JavaDoc Tag Summary]
+
 
+
=== Documentation Requirements ===
+
 
+
API documentation must be written to conform to the guidelines set out in Sun's [http://java.sun.com/j2se/javadoc/writingapispecs/index.html Requirements for Writing API Specifications]. This document is full of useful examples from the JDK.
+
 
+
In addition to the requirements from this document all types, and methods where relevant, must document their concurrent behavior. Types should document their concurrent semantics including whether or not they are thread safe.
+
 
+
==== Documenting Interface Concurrency ====
+
 
+
Interfaces must document any restrictions that their contract places on the concurrency of an implementation. For example, an interface may require that a Collection returned from one of its methods must reflect internal updates accurately or must throw ConcurrentModificationException if a change occurs during iteration. For example:
+
 
+
<pre>
+
/**
+
* ...
+
*
+
* <strong>Concurrent Semantics</strong><br/>
+
*
+
* Implementations must be thread safe.
+
*/
+
public interface ...
+
</pre>
+
 
+
==== Documenting Type-Level Concurrency ====
+
 
+
Types must as a minimum document whether or not they are thread safe. In addition, types must document any lock acquisition protocols that they require calling code to adhere to.
+
 
+
Also, types must document the guarantees around state visibility that they provide as part of their implemented concurrency level. For example, some types may present internal Collection state in a weakly consistent manner.
+
 
+
Example:
+
<pre>
+
/**
+
* ...
+
*
+
* <strong>Concurrent Semantics</strong><br/>
+
*
+
* Implementation is immutable and therefore thread safe.
+
*
+
*/
+
</pre>
+
 
+
==== Documenting Method-Level Concurrency ====
+
 
+
For methods that have specific concurrency semantics, these semantics should be included in the JavaDoc. For example:
+
 
+
<pre>
+
/**
+
* @inheritDoc
+
* <p/>
+
* Returns a weakly consistent view of the <code>Subsystems</code>.
+
* This is guaranteed to be consistent with the view of <code>Subsystems</code>
+
* when this method is called. Concurrent modifications are not guaranteed to be
+
*  reflected.
+
* @see ConcurrentHashMap#values()
+
*
+
*/
+
public Collection<Subsystem> getSubsystems() {
+
    ...
+
}
+
</pre>
+
  
 
= Testing =  
 
= Testing =  

Revision as of 12:06, 7 December 2010


Committers Charter

Committers are responsible for:

  • Maintaining and enhancing the Virgo code base
  • Responding to queries on virgo-dev
  • Handling contributions
  • Developing contributors into committers
  • Complying with Eclipse IP policy
  • Keeping an eye on the Virgo forum

Machine Setup

You need Sun JDK 6, Apache Ant 1.7.1 or later, and git. You'll probably want an IDE such as Eclipse.

At the time of writing, some ant targets occasionally fail because they cannot load classes from jsch-0.1.42.jar. A workaround on Mac OS X is to copy this JAR from virgo-build's /lib/ivy directory to /opt/local/share/java/apache-ant/lib.

On Mac OS X, increase the default number of concurrently open files by adding this to .profile:

       ulimit -n 10000

To run certain scripts, you'll need ruby, gems, and the 'choice' gem. On Mac OS you can get these by installing the XCode tools (from the Mac OS X disk) and MacPorts, then issuing:

       sudo port -v install ruby
       sudo port -v install rb-rubygems 
       sudo gem install --remote choice

Coding

Virgo has a strong emphasis on maintainable, or "clean", code. If you need a really good introduction to coding, we recommend "Code Complete". If you are already a proficient programmer and want to write really clean code, read "Clean Code". If you are not an expert in writing Java, read "Effective Java".

Virgo code is thread safe unless specifically stated. Achieving thread safety in Java is not easy. We recommend "Java Concurrency in Practice" for a good grounding in Java concurrency.

Virgo/Committers/Coding

Testing

See Test.

IP Due Diligence

Handling Code Contributions

Committers are responsible for ensuring that Eclipse IP policy, summarised in the legal poster, is adhered to. Patches must be attached to a bugzilla bug, the bug must have its iplog flag set to '+', and the contributor must confirm in the bug that: they wrote 100% of the code, they have the right to contribute the code to Eclipse, and new Java file headers contain the appropriate License header.

Raising "works with" CQs

"works with" CQs are required for test dependencies which are not distributed with Virgo or checked in to Eclipse version control (git, svn, cvs).

The initial set of test dependencies was determined, for repositories which are built with ant, as follows:

1. In the build-xxx directory run ant report

2. Extract a raw list of the test dependency jars

find <report directory> -name "*-test.xml" -exec grep -E \/.+\.jar  {} \; >test-jars.txt

3. Edit test-jars.txt using an editor with a regular expression global change facility and do the following global changes.

3.1 Replace regex .+\/(.+)\.jar\"> with $1.jar.

3.2 Replace regex .+\/(.+)\.jar\"\/> with $1.jar.

3.3 Replace -sources with the empty string

4. Sort test-jars.txt and remove duplicate lines.

5. Look through test-jars.txt and raise "works with" for any JARs which don't have a Virgo CQ for the correct version. Also, raise corresponding Virgo bugzillas and set the iplog flag to enter the bugzillas in the automated IP log.