river-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dan Creswell <dan.cresw...@gmail.com>
Subject Re: VOTE: add Startable to Jini specification
Date Thu, 19 Dec 2013 21:48:49 GMT
"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 <trasukg@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 <jini@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 <jini@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 <jini@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 <jini@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 <jini@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.
> >>>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>>
> >>
> >
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message