Hopefully Startable will get enough votes to get across the line, we need to
make progress, if you care about River you'll vote for it, not because it's the
perfect solution, simply because it's an acceptable solution that makes sense
and it marks a progress milestone.
The reflective constructor signature is acceptable and is used by the starter
kit and Activation.
We've got Groovy Configuration.
The service class can also define a static main method.
If we at least accept these signatures for our services, they could run in any
container.
Perhaps you also want a public shutdown method for lifecycle management?
Closeable would also be acceptable?
Is there anything else you need?
Regards,
Peter.
----- Original message -----
> "What we need is a discussion on a more developer-friendly service
> startup convention to replace ServiceStarter."
>
> Maybe, that is to say, I'm not anti but I feel there's an additional
> wider aspect to kickaround as well....
>
> Should there be a failure of the type we've discussed so far, where does
> the responsibility for sorting it out lie? The service writer, they
> created the bug in the first place. BUT, has it occurred to anyone that
> export() isn't the only place multi-threading problems can occur in a
> service implementation?
>
> Even if they've got service construction right (with whatever convention
> or pattern you care to adopt, me, I'm in the camp of if it works because
> it accounts for the JMM it's fine) have they got all their
> multi-threading right elsewhere? What about all those service methods
> exported and advertised by JoinManager?
>
> Do we actually think it's possible to address that challenge? (You could
> do it by single-locking the entire service so only one call can be
> dispatched at a time, yeah right, that's palatable). What makes export a
> special case in this context? Why does that matter so when there are
> many other ways to screw up?
>
>
> On 19 December 2013 20:16, Greg Trasuk <tras...@stratuscom.com> wrote:
>
> >
> > +0. Let me explain…
> >
> > Having a separate interface to say start() is all very well and
> > harmless in itself, but:
> >
> > (a) It doesn’t really address the issue of how service instances should
> > be started. We don’t actually have solid recommendations on this
> > issue, although there have been attempts in various containers to
> > systematize it.
> >
> > (b) the proposed warning addresses a case where a service instance is
> > doing “exporter.export(this);”, which I’d call bad practice in any
> > case. It happens that ServiceStarter appears to encourage this use
> > case because it works by calling a constructor. But the called
> > constructor could (and should) construct an instance of some other
> > class, and then export and register that instance rather than “this”.
> > That style doesn’t mandate a Startable interface, and having Startable
> > doesn’t mandate good multi-threaded code. In other words, you could
> > implement Startable and still have poor thread-safety.
> >
> > (c) even where there is an apparent problem, the exposure window is
> > very small. Consider the last bits of the init(…) method in
> > “com.sun.jini.reggie.ServiceRegistrarImpl”
> >
> > myRef = (Registrar) serverExporter.export(this);
> > proxy = RegistrarProxy.getInstance(myRef, myServiceID);
> > myLocator = (proxy instanceof RemoteMethodControl) ?
> > new ConstrainableLookupLocator(
> > unicastDiscoveryHost, unicaster.port, null) :
> > new LookupLocator(unicastDiscoveryHost, unicaster.port);
> > /* register myself */
> > Item item = new Item(new ServiceItem(myServiceID,
> > proxy,
> > lookupAttrs));
> > SvcReg reg = new SvcReg(item, myLeaseID, Long.MAX_VALUE);
> > addService(reg);
> > if (log != null) {
> > log.snapshot();
> > }
> >
> > try {
> > DiscoveryGroupManagement dgm = (DiscoveryGroupManagement)
> > discoer;
> > String[] groups = dgm.getGroups();
> > if (groups == null || groups.length > 0) {
> > throw new ConfigurationException(
> > "discoveryManager must be initially configured with " +
> > "no groups");
> > }
> > DiscoveryLocatorManagement dlm =
> > (DiscoveryLocatorManagement) discoer;
> > if (dlm.getLocators().length > 0) {
> > throw new ConfigurationException(
> > "discoveryManager must be initially configured with " +
> > "no locators");
> > }
> > dgm.setGroups(lookupGroups);
> > dlm.setLocators(lookupLocators);
> > } catch (ClassCastException e) {
> > throw new ConfigurationException(null, e);
> > }
> > joiner = new JoinManager(proxy, lookupAttrs, myServiceID,
> > discoer, null, config);
> >
> > /* start up all the daemon threads */
> > serviceExpirer.start();
> > eventExpirer.start();
> > unicaster.start();
> > multicaster.start();
> > announcer.start();
> >
> > /* Shutdown hook so reggie sends a final announcement
> > * packet if VM is terminated. If reggie is terminated
> > * through DestroyAdmin.destroy() this hook will have no effect.
> > * A timeout on announcer.join() was considered but not deemed
> > * necessary at this point in time.
> > */
> > Runtime.getRuntime().addShutdownHook(new Thread( new Runnable() {
> > public void run() {
> > try {
> > announcer.interrupt();
> > announcer.join();
> > } catch (Throwable t) {
> > logThrow(Level.FINEST, getClass().getName(),
> > "run", "exception shutting announcer down",
> > new Object[]{}, t);
> > }
> > }
> > }));
> >
> > snapshotter.start();
> > if (logger.isLoggable(Level.INFO)) {
> > logger.log(Level.INFO, "started Reggie: {0}, {1}, {2}",
> > new Object[]{ myServiceID,
> > Arrays.asList(memberGroups),
> > myLocator });
> > }
> > ready.ready();
> > }
> >
> >
> > The window from when the join manager is started up (ignore the
> > serverExporter.export(). The only way a client gets to this endpoint
> > is through a proxy downloaded from the registrar) to when the init()
> > method exits is effectively 9 lines of code, during which time a _lot_
> > of network i/o would have to happen in order for the object to be used
> > prior to the constructor exit. As well, the actual remote calls are
> > filtered through a command interface and through the tasker object,
> > and the command classes often go through synchronized() blocks, which
> > (I think) act as happens-before fences.
> >
> > The whole thing seems to me a little like the old servlet days, when we
> > got the SingleThreadModel interface and it only served to give naive
> > developers a false sense of security.
> >
> > What we need is a discussion on a more developer-friendly service
> > startup convention to replace ServiceStarter.
> >
> > Having said that, I’m not overly opposed to having Startable, given
> > that as Dan suggested, any output from the ServiceStarter is in the
> > form of a suggestion rather than an error.
> >
> > Cheers,
> >
> > Greg.
> >
> >
> >
> > On Dec 18, 2013, at 7:42 PM, Peter <j...@zeus.net.au> wrote:
> >
> > > +1 Peter.
> > >
> > >
> > > I've been going over the Exporter implementations and Exporter
> > > interface.
> > >
> > > The Exporter interface specifies that export and unexport behaviour
> > > is
> > defined by the implementation.
> > >
> > > That means it's up to the implementation to specify whether a happens
> > before edge occures before export.
> > >
> > > I propose we document in Exporter, that if export is called during
> > object construction then that object's implementation cannot safely use
> > final fields.
> > >
> > > We should also provide a @see reference to Startable to reccommend
> > > its
> > use when final fields are desirable and include Startable in the spec.
> > >
> > > We don't need to rewrite the example of exporting in net.jini.config
> > > as
> > export isn't performed during construction.
> > >
> > > This is a minor update to the Jini Specification.
> > >
> > > The FINE log message is an implementation detail and won't be
> > > included.
> > >
> > > Regards,
> > >
> > > Peter.
> > >
> > > ----- Original message -----
> > > > No, an effectively immutable field is one whose reference doesn't
> > > > change after the service objects safe publication.
> > > >
> > > > An immutable field is final.
> > > >
> > > > It would be Jeri's responsibility to safely publish the service
> > > > during export to ensure a happens before event occurs, ensuring
> > > > all non volatile fields the service contains are safely published
> > > > and visible.
> > > >
> > > > It's possible to have mutable fields guarded by synchronized
> > > > methods, or volatile fields.
> > > >
> > > > But it isn't possible to have final fields when exporting within
> > > > the constructor.
> > > >
> > > > If the service contains effectively immutable field references, it
> > > > must be safely published or if another thread accesses those
> > > > fields through unsynchronized methods, the thread may only see
> > > > their default value.
> > > >
> > > > All our service implementations have final fields. All our
> > > > services
> > had
> > > > some unsynchronized access to non volatile, non final fields.
> > > >
> > > > Trouble is, when you can't have final fields, it's very difficult
> > > > to determine which fields are effectively immutable and the
> > > > original programmers design intent, if there's no comment.
> > > >
> > > > Our service implementations all had unsynchronized init() methods
> > > > called from with the constructor that started threads and exported
> > > > Some
> > fields
> > > > were final.
> > > >
> > > > For me, a service that contains final fields and is exported after
> > > > construction is easier to maintain and understand.
> > > >
> > > > I suspect, like our Jini service imlementations, most people get it
> > > > wrong most of the time.
> > > >
> > > > I get that others won't want to use this method, however it would
> > > > be detrimental if we don't have such a method for those who would.
> > > >
> > > > Regards,
> > > >
> > > > Peter.
> > > >
> > > > ----- Original message -----
> > > > >
> > > > > I feel like we’re going down a rabbit hole here when you start
> > > > > talking about exporting immutable objects. Wouldn’t it be
> > > > > kind of silly to export an immutable service? Isn’t the whole
> > > > > point that you interact with the service (i.e. alter its state)
> > > > > over a remote interface?
> > > > >
> > > > > Perhaps it’s better to say that exported services need to be
> > > > > thread-safe (which should be fairly obvious). Yes, immutable
> > > > > objects are inherently thread-safe, so for sharing information
> > > > > inside a VM, it makes some sense to minimize the number of
> > > > > mutable objects you pass around. But fundamentally, we’re
> > > > > talking about shared-state systems here.
> > > > >
> > > > > Cheers,
> > > > >
> > > > > Greg.
> > > > >
> > > > > On Dec 18, 2013, at 7:42 AM, Peter <j...@zeus.net.au> wrote:
> > > > >
> > > > > > Pat your comment about non final fields is interesting.
> > > > > >
> > > > > > Isn't it also the case that we need to safely publish an
> > > > > > effectively immutable object to share it among threads?
> > > > > > That usually means copying it to a thread safe collection or
> > > > > > shared via a synchronized method, volatile field, or final
> > > > > > field in another object?
> > > > > >
> > > > > > So we should also make sure that Jeri uses safe publication
> > > > > > during export.
> > > > > >
> > > > > > That would allow a service that has no final fields to start
> > > > > > threads, then export from within a constructor safely,
> > > > > > provided all operations on non final fields happen before
> > > > > > starting threads and exporting.
> > > > > >
> > > > > > All our services have final fields, so Starter is more
> > > > > > appropriate for River's own services.
> > > > > >
> > > > > > Regards,
> > > > > >
> > > > > > Peter.
> > > > > >
> > > > > > ----- Original message -----
> > > > > > > Hmm, good point, Startable, makes more sense.
> > > > > > >
> > > > > > > An object can be exported using Startable.
> > > > > > >
> > > > > > > I think we should have a policy to strongly discourage
> > > > > > > exporting from constructors.
> > > > > > >
> > > > > > > Regards,
> > > > > > >
> > > > > > > Peter.
> > > > > > >
> > > > > > > ----- Original message -----
> > > > > > > > As far as I can tell, the special properties of completing
> > > > > > > > a constructor in the JLS memory model are:
> > > > > > > >
> > > > > > > > 1. A happens-before edge from the end of the constructor
> > > > > > > > to the start of a finalizer. (17.4.5)
> > > > > > > >
> > > > > > > > 2. The guarantee that any thread that only sees a
> > > > > > > > reference to an object after the end of the
> > > > > > > > constructor will see the correctly initialized values
> > > > > > > > of all final fields. (17.5)
> > > > > > > >
> > > > > > > > The special issue with final fields is that
> > > > > > > > implementations have freedom to optimize access to
> > > > > > > > final fields in ways that are not permitted for
> > > > > > > > non-final fields. Strategies for thread safety that work
> > > > > > > > for non-final fields do not necessarily work for
> > > > > > > > final fields. The requirement for final field safety
> > > > > > > > is that the constructor end before another thread accesses
> > > > > > > > the newly constructed object.
> > > > > > > >
> > > > > > > > Calling a start() method after construction if the class
> > > > > > > > implements a new interface seems to me to be harmless,
> > > > > > > > backwards compatible, and useful. It enables the
> > > > > > > > simplest and most direct way of preventing access to
> > > > > > > > the new object by another thread during construction.
> > > > > > > >
> > > > > > > > The roadmap issue is whether it should be required, and if
> > > > > > > > so the level of enforcement. For example, there is no
> > > > > > > > reason to require it if the class does not declare
> > > > > > > > any final fields.
> > > > > > > >
> > > > > > > > Incidentally, and as a detail, "Commission" does not
> > > > > > > > immediately make me think of having a start() method
> > > > > > > > that should be called after construction. If you do go
> > > > > > > > this way, the name needs thought. "Startable" would
> > > > > > > > be more obvious, more memorable, more likely to be found
> > > > > > > > on searches, and more compatible with familiar
> > > > > > > > interface names such as "Cloneable" and "Iterable".
> > > > > > > >
> > > > > > > > Patricia
> > > > > > > >
> > > > > > > >
> > > > > > > > On 12/18/2013 2:18 AM, Peter wrote:
> > > > > > > > > Well, now seems like a good time to have the
> > > > > > > > > conversation.
> > > > > > > > >
> > > > > > > > > Yes there are other ways, but I haven't seen one safe
> > > > > > > > > implementation yet, so...
> > > > > > > > >
> > > > > > > > > Does someone have a better way to solve this problem, has
> > > > > > > > > someone already solved this problem I'm unaware of that
> > > > > > > > > we can adopt, or is there a way that's more satisfactory?
> > > > > > > > >
> > > > > > > > > If not, is there something objectionable with the
> > > > > > > > > Commission interface and if so, how can we fix it?
> > > > > > > > >
> > > > > > > > > The SEVERE log message is logged by the River start
> > > > > > > > > package, other containers or frameworks can choose
> > > > > > > > > whether or not to do so, but I'd encourage them to do
> > > > > > > > > something similar, yes we can change it to WARN.
> > > > > > > > >
> > > > > > > > > A much harsher option is to throw an exception during
> > > > > > > > > export which breaks backward compatibility.
> > > > > > > > >
> > > > > > > > > Regards,
> > > > > > > > >
> > > > > > > > > Peter.
> > > > > > > > >
> > > > > > > > > ----- Original message -----
> > > > > > > > > > "org.apache.river.api.util.Commission is an interface
> > > > > > > > > > services should implement"
> > > > > > > > > >
> > > > > > > > > > If it's a SHOULD, not a MUST, chucking out a SEVERE is
> > > > > > > > > > incorrect logger behaviour IMO. You could issue a WARN
> > > > > > > > > > if you like but for even that I'd say you need to
> > > > > > > > > > provide a roadmap explaining why the warning and what
> > > > > > > > > > you intend to do in future and what you expect of
> > > > > > > > > > service writers such as myself.
> > > > > > > > > >
> > > > > > > > > > Commission, at least from my point of view, is your
> > > > > > > > > > means (maybe the River community's - did you ask us?)
> > > > > > > > > > for satisfying your needs in respect of the JMM. As
> > > > > > > > > > we've discussed previously, there are other ways too
> > > > > > > > > > and they work and they are safe if you know what
> > > > > > > > > > you're doing. Your contention was that most don't know
> > > > > > > > > > what they're doing hence, presumably, Commission.
> > > > > > > > > >
> > > > > > > > > > So the thing is, you are seemingly on a road to
> > > > > > > > > > asserting more structure (gosh, a standard?) on the
> > > > > > > > > > way people write their services. If so, you'd best
> > > > > > > > > > start flagging that honestly and openly via a roadmap,
> > > > > > > > > > deprecation and such/whatever rather than sticking out
> > > > > > > > > > logger messages with no clear guidance and at the cost
> > > > > > > > > > of a certain amount of nuisance (no admin I know likes
> > > > > > > > > > SEVERE's being logged for something which isn't
> > > > > > > > > > critical cos it's noise they don't want in log files).
> > > > > > > > > >
> > > > > > > > > > And of course, we all know that when some entity
> > > > > > > > > > asserts a standard or requirement on others for entry,
> > > > > > > > > > they may choose not to enter. Does this help your
> > > > > > > > > > community or hinder it? The answer to that is, it
> > > > > > > > > > depends. On what? Have you asked or tested? How have
> > > > > > > > > > you tested? What would be considered validation or
> > > > > > > > > > lack of support?
> > > > > > > > > >
> > > > > > > > > > I am not out to flame or troll rather I want to see
> > > > > > > > > > this community demonstrating good behaviour and I'm
> > > > > > > > > > not feeling like what's going on around Commission
> > > > > > > > > > (what is that big change in version number really
> > > > > > > > > > saying?) is such.
> > > > > > > > > >
> > > > > > > > > > On 18 December 2013 08:52, Peter <j...@zeus.net.au>
> > > > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > > Just to clarify org.apache.river.api.util.Commission
> > > > > > > > > > > is an interface services should implement, I would
> > > > > > > > > > > encourage all container projects to pick up the
> > > > > > > > > > > interface and make suggestions for improvement if
> > > > > > > > > > > there are any issues.
> > > > > > > > > > >
> > > > > > > > > > > Interface Commission {
> > > > > > > > > > > void start () throws Exception;
> > > > > > > > > > > }
> > > > > > > > > > >
> > > > > > > > > > > It's called after JMM safe construction to allow the
> > > > > > > > > > > service to start any threads and be exported.
> > > > > > > > > > >
> > > > > > > > > > > Regards,
> > > > > > > > > > >
> > > > > > > > > > > Peter.
> > > > > > > > > > >
> > > > > > > > > > > ----- Original message -----
> > > > > > > > > > > >
> > > > > > > > > > > > The way that services are instantiated and setup
> > > > > > > > > > > > is an implementation detail.
> > > > > > > > > > > >
> > > > > > > > > > > > When I think of compatibility I think of the API
> > > > > > > > > > > > and the lookup methods. We think of compatibility
> > > > > > > > > > > > from a client point of view.
> > > > > > > > > > > >
> > > > > > > > > > > > From the client point of view, using a service
> > > > > > > > > > > > looks like this:
> > > > > > > > > > > >
> > > > > > > > > > > > - Use multicast of unicast discovery to find one
> > > > > > > > > > > > or more ServiceRegistrar instances
> > > > > > > > > > > >
> > > > > > > > > > > > - Call lookup(…) on one or more of
> > > > > > > > > > > > these instances to get a set of service candidates
> > > > > > > > > > > > - Choose a candidate and prepare() it using a
> > > > > > > > > > > > ProxyPreparer, to yield a usable service proxy.
> > > > > > > > > > > > - Make
> > > > > > > > > > > >calls on it.
> > > > > > > > > > > > Ideally hang on to this proxy
> > > > > > > > > > > > instance, so you can skip the discovery and lookup
> > > > > > > > > > > > next time you need it.
> > > > > > > > > > > >
> > > > > > > > > > > > - If the call fails, repeat the lookup (and
> > > > > > > > > > > > possibly discovery) til you get a proxy that works.
> > > > > > > > > > > >
> > > > > > > > > > > > Nowhere does the client need to know whether the
> > > > > > > > > > > > service instance is started up using the
> > > > > > > > > > > > “com.sun.jini.start” mechanism, your Commission
> > > > > > > > > > > > interface, some other IOC container (Rio,
> > > > > > > > > > > > Harvester, Seven or RiverContainer) or some
> > > > > > > > > > > > unknown mechanism that starts with a static main()
> > > > > > > > > > > > method.
> > > > > > > > > > > >
> > > > > > > > > > > > JSK2.0 was 2.0 because of the introduction of the
> > > > > > > > > > > > proxy verification mechanisms, as well as JERI.
> > > > > > > > > > > > Absent some new client usage mechanism, River
> > > > > > > > > > > > doesn’t need to go to 3.0.
> > > > > > > > > > > >
> > > > > > > > > > > > Cheers,
> > > > > > > > > > > >
> > > > > > > > > > > > Greg.
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > On Dec 17, 2013, at 1:58 PM, Peter
> > > > > > > > > > > > <j...@zeus.net.au> wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > > I think changing services to use safe
> > > > > > > > > > > > > construction techniques is enough to cause the
> > > > > > > > > > > > > version jump.
> > > > > > > > > > > > >
> > > > > > > > > > > > > At this point I've allowed services to continue
> > > > > > > > > > > > > unsafe construction practices, while logging a
> > > > > > > > > > > > > SEVERE warning when the Commission interface
> > > > > > > > > > > > > isn't implemented, rather than fail.
> > > > > > > > > > > > >
> > > > > > > > > > > > > This is a fundamental change to the way services
> > > > > > > > > > > > > are written.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Regards,
> > > > > > > > > > > > >
> > > > > > > > > > > > > Peter.
> > > > > > > > > > > > >
> > > > > > > > > > > > > ----- Original message -----
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Assuming that there aren’t major
> > > > > > > > > > > > > > incompatibilities, I think that would be a
> > > > > > > > > > > > > > “minor” version change according to our
> > > > > > > > > > > > > > versioning policy, so we’d be looking at the
> > > > > > > > > > > > > > “2.3” branch rather than a “3.0” release.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > I’m still unnerved by the massive amounts of
> > > > > > > > > > > > > > changes to both code and tests in the
> > > > > > > > > > > > > > qa_refactor branch, as well as the apparent
> > > > > > > > > > > > > > instability of the code, although that seems
> > > > > > > > > > > > > > to be improving. In the next few weeks I’m
> > > > > > > > > > > > > > going to try and setup a cross-test case, to
> > > > > > > > > > > > > > see what the “2.2” tests say about the
> > > > > > > > > > > > > > potential “2.3” release and vice-versa.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > I think what I’d really like to see is an
> > > > > > > > > > > > > > incremental approach where we update limited
> > > > > > > > > > > > > > components of the “2.2” branch, one at a time.
> > > > > > > > > > > > > > Is there anything that we could pull out
> > > > > > > > > > > > > > piecemeal? Maybe it
> > > > > > > > > > > would
> > > > > > > > > > > > > > make sense to split out the infrastructure
> > > > > > > > > > > > > > services, like Reggie, Mahalo, and Outrigger
> > > > > > > > > > > > > > into different sub-projects that could be
> > > > > > > > > > > > > > updated separately?
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Any thoughts?
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Greg.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On Dec 17, 2013, at 5:03 AM, Peter
> > > > > > > > > > > > > > <j...@zeus.net.au> wrote:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > > When the qa_refactor branch stabilises, I
> > > > > > > > > > > > > > > plan to merge trunk and provide a beta
> > > > > > > > > > > > > > > release for client compatibility testing.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Changes made have been focused on making our
> > > > > > > > > > > > > > > code thread safe, there are significant
> > > > > > > > > > > > > > > changes internally, the public api remains
> > > > > > > > > > > > > > > focused on backward compatibility, however
> > > > > > > > > > > > > > > it is advisable that client services adopt
> > > > > > > > > > > > > > > new safe construction techniques for
> > > > > > > > > > > > > > > services and implement the new Commission
> > > > > > > > > > > > > > > interface.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > What's a suitable test period for client
> > > > > > > > > > > > > > > testing?
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Regards,
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Peter.
> > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> >
