users@ejb-spec.java.net

[ejb-spec users] Re: [cdi-dev] Fwd: Interceptors spec 1.2 draft is available for review

From: Mark Struberg <struberg_at_yahoo.de>
Date: Mon, 4 Feb 2013 09:53:36 +0000 (GMT)

> Nothing can be destroyed during the preDestroy callstack. In particular
> fields declared in the superclass. All injected references should be
> also there.

Why not? That's nowhere defined that way! I've seen this tons of times in productive code and there is no restriction in any spec I know which states 'You must not set a field to null in_at_PreDestroy'.


It also depends on how you coin the term 'destroy'. Freeing a resource imo also counts as 'destroying' (e.g. calling entityManager.close() if you have an application managed EM). And that is what happens predominantly in @PreDestroy. Thus if you release a manually managed resource in the parent class (maybe even set it to null) and try to access it from the subclass, then you will blow up.



> be created before the PostConstruct call chain is started, and say that
> no instances are destroyed until the PreDestroy chain has finished.

The important part you need to add here (at least in your thoughts) is "by the container"!
@PostConstruct and @PreDestroy are explicitly there for manually managed resources and state!

LieGrue,
strub


----- Original Message -----
> From: Marina Vatkina <marina.vatkina_at_oracle.com>
> To: users_at_ejb-spec.java.net
> Cc:
> Sent: Monday, February 4, 2013 2:17 AM
> Subject: [ejb-spec users] Re: [cdi-dev] Fwd: Interceptors spec 1.2 draft is available for review
>
> On 2/2/13 2:16 PM, Mark Struberg wrote:
>>>>   Unfortunately @PreDestroy in RI (GlassFish) is called in the
>>> "defined" order, i.e. the same order as @PostConstruct. Bill
> brought
>>> this up at the beginning of EE7, but because it wasn't defined when
>>> introduced, it's too late to change. So all interceptors are
> invoked
>>> according to the same rule.
>>>>   Does CDI behave differently?
>>> I'm pretty sure CDI does it the way mark describes. But I woulld
> have to
>>> check.
>> Well, at least in OpenWebBeans we do it subclass first for @PreDestroy ever
> since if I remember correctly. At least since a long time now, and we never had
> issues with any TCK.
>
> It might not have a good TCK coverage...
>
>>
>> The point is: parent-class-first for any destroyal operation would limit
> the functionality heavily. You e.g. can only touch fields and methods on your
> current class-level but not of any parent class as they might have been
> destroyed already.
>
> Nothing can be destroyed during the preDestroy callstack. In particular
> fields declared in the superclass. All injected references should be
> also there.
>
> I'd rather be symmetrical with the requirement that all instances must
> be created before the PostConstruct call chain is started, and say that
> no instances are destroyed until the PreDestroy chain has finished.
>
> -marina
>
>> Thus by defining this as subclass-first you only gain functionality but
> don't loose any, right? Or did I overlook anything? Today you would need to
> implement the @PreDestroy only in the uppermost parent and call a virtual method
> which does subclass first. But even here it would be no problem to switch to
> subclass first for @PreDestroy methods as they are not implemented in the
> subclasses anyway...
>>
>>
>> So in my opinion we should define this for EE7 as this looks a perfectly
> backward compatible change for me. Of course this needs a review and other
> brains who check that we didn't overlook something.
>>
>>
>> Replies to the other parts will follow tomorrow.
>>
>>
>> LieGrue,
>> strub
>>
>>
>> ----- Original Message -----
>>> From: Pete Muir <pmuir_at_redhat.com>
>>> To: marina.vatkina_at_oracle.com
>>> Cc: Mark Struberg <struberg_at_yahoo.de>; ejb-users
> <users_at_ejb-spec.java.net>
>>> Sent: Saturday, February 2, 2013 11:45 AM
>>> Subject: Re: [ejb-spec users] [cdi-dev] Fwd: Interceptors spec 1.2
> draft is available for review
>>>
>>>
>>> On 2 Feb 2013, at 03:29, Marina Vatkina wrote:
>>>
>>>>   Hi Mark,
>>>>
>>>>   Thank you for the review!
>>>>
>>>>   On 1/31/13 1:41 PM, Mark Struberg wrote:
>>>>>   Hi Marina, Pete, other EG fellows!
>>>>>
>>>>>   I quickly read through the spec and took some small notes.
> Some points
>>> maybe right, others might be wrong, hope the feedback helps nontheless
> ;)
>>>>>
>>>>>   Chapter 2.1:
>>>>>   "Around-invoke interceptor methods, around-timeout
> interceptor
>>> methods, and lifecycle callback interceptor methods may be defined on
> the same
>>> interceptor class."
>>>>>   This sounds like there could be multiple of each in the same
> class,
>>> e.g. 2 AroundInvoke. I know this is clarified later, but maybe we can
> find a
>>> wording which makes this more clear from the beginning. It's also
> often
>>> unclear if 'on the same interceptor class' includes parent
> classes or
>>> not. Maybe add a link to a later paragraph where this is defined much
> more in
>>> detail.
>>>>   How about this:
>>>>
>>>>   "An interceptor class may define an interceptor method for
> all
>>> interposed invocations: an around-invoke interceptor method,
> around-timeout
>>> interceptor method, and a lifecycle callback interceptor method for
> each
>>> lifecycle event. See section xxx on invocation order of interceptors
> with
>>> superclasses."
>>>>   Or this"
>>>>
>>>>   "An around-invoke interceptor method, around-timeout
> interceptor
>>> method, and lifecycle callback interceptor methods for different
> lifecycle
>>> events may be defined on the same interceptor class. See section xxx on
>>> invocation order of interceptors with superclasses."
>>>>>
>>>>>   Chapter 2.2:
>>>>>   Maybe add that @PostConstruct lifecycle interceptors are
> invoked in
>>> parent-class-first order, whereas @PreDestroy interceptors are invoked
> in
>>> subclass-first order. Or is this defined elsewhere already? They should
> follow
>>> constructor invocation order respective finalize order. It might also
> be ok to
>>> clarify that overloaded interceptor methods in subclasses disable their
>>> respective information from the parent class. Even introduce an own
>>> 'Interceptor Method resolution' chapter or similar?
>>>>   Unfortunately @PreDestroy in RI (GlassFish) is called in the
>>> "defined" order, i.e. the same order as @PostConstruct. Bill
> brought
>>> this up at the beginning of EE7, but because it wasn't defined when
>>> introduced, it's too late to change. So all interceptors are
> invoked
>>> according to the same rule.
>>>>   Does CDI behave differently?
>>> I'm pretty sure CDI does it the way mark describes. But I woulld
> have to
>>> check.
>>>
>>>>>   Chapter 2.5:
>>>>>   "An around-invoke method must not be declared as
> abstract, final
>>> or static."
>>>>>   Why so? I can see that abstract makes no sense, buy we
> don't need
>>> to further proxy them, don't we? So why not final nor static? Has
> this to do
>>> with some EJB limitations?
>>>>   Yes, it was an EJB limitation when interceptors were added. Is it
> a problem
>>> for CDI?
>>>
>>> No, it's just uncessary. We could relax this.
>>>
>>>>>   "Only one around-invoke method may be defined on a given
>>> class."
>>>>>   Including superclasses or not? Would it be ok to have 1
> @AroundInvoke
>>> in the subclass and another in the parent class or max 1 for the whole
> class
>>> hierarchy? This is in general not always clear from the wording. I
> remember
>>> having asked this a few years ago and since then we support 1 per each
> class in
>>> the hierarchy in OWB.
>>>>   It's there in the ordering chapter, where I just created a
> special
>>> section from this bullet:
>>>>   "If an interceptor class itself has superclasses, the
> interceptor
>>> methods defined by the interceptor class’s superclasses are invoked
> before the
>>> interceptor method defined by the interceptor class, most general
> superclass
>>> first."
>>>>   Do you think I should add a ref to each type explicitly?
>>> I think this needs to be explict in the definitions as well.
>>>
>>>>>   Chapter 2.8:
>>>>>   "For example, an around-invoke interceptor method may be
> defined
>>> to apply only to a specific target class method invocation,.."
>>>>>   Why 'for example'? A lifecycle interceptor cannot be
> annotated
>>> to some method, only @AroundInvoke can, isn't? To be honest I
> didn't get
>>> the paragraph about method-level timeout interceptors. Maybe it's
> just me
>>> understanding the term the wrong way. Can you give me a sample of a
> timeout
>>> interceptor applied to a method?
>>>>   There can be multiple timeout methods defined via @Schedule. Each
> one can
>>> have its own set of interceptors (strange, but possible). I can remove
> "For
>>> example," if you feel it's confusing.
>>>>>   just formatting:
>>>>>   page 14 is intentionally left blank?
>>>>   Yes. It follows EJB spec format - each chapter starts on the
> right page of
>>> an open book.
>>>>>   page 15/ Chapter3 has a huge empty block as well.
>>>>   Do you mean at the top? Every chapter starts at the bottom of its
> page.
>>>>
>>>>>   Chapter 3.2:
>>>>>   "If an interceptor does not declare an Interceptor
> annotation, it
>>> could be bound to components using Interceptors annotation or a
> deployment
>>> descriptor file."
>>>>>   Does this also imply forbidding the reverse? That a class
> annotated
>>> with @Interceptor must not be used in @Interceptors(..) ?
>>>>   I'm not sure we should this should be forbidden, though ch2
> says
>>> "Interceptors classes may be associated with the target class
> using either
>>> interceptor binding (see Chapter 3) or the Interceptors annotation (see
> Chapter
>>> 4)."
>>> I don't think it does imply the reverse.
>>>
>>>>>   This chapter should also note that the rules for activation
> such
>>> @Interceptor classes might be defined in other specs (e.g. for CDI only
> if
>>> enabled in a beans.xml). Or we define those rules here.
>>>>   May be ch5 should be called "Enabling and Ordering
> Interceptors"?
>>> Agree with Mark. Let's make sure this extensible.
>>>
>>>>>   Otherwise an @Interceptor which is not enabled in beans.xml
> would _not_
>>> be active for CDI beans but enabled for EJBs? And what happens if you
> have a
>>> @RequestScoped @Stateful class SomeBean?
>>>>   If it's enabled via @Interceptors on the @Stateful SomeBean,
> it will be
>>> called as all other "EJB" interceptor.
>>>
>>> Right, EJB on it's own doesn't support interceptor bindings.
>>>
>>>>>   Chapter 3.4:
>>>>>   This chapter should generally introduce the restriction on
> 'active
>>> interceptors'.
>>>>>   This chapter defines the behavior of @Nonbinding. Does this
> mean we
>>> need to move this annotation from the CDI spec packages (e.g.
> cdi-api.jar) to
>>> the interceptor spec API jar? Or better to commons-annotations?
>>>>   Pete though we would, but we can't move an annotation to a
> different
>>> package without breaking backward compatibility, and other members of
> that
>>> package are CDI-specific. The version that I'm working on refers to
> the CDI
>>> spec on @Nonbinding rules.
>>>
>>> We would need to move the whole package (javax.enterprise.util) to
> common
>>> annotations. The other members of the package is are NOT CDI specific,
> and I
>>> asked the Java EE spec leads to consider moving the package, but it was
> turned
>>> down (not sure why exactly). We do have other specs who would like this
> package
>>> moved (JAX-RS).
>>>
>>>>>   Chapter 3.4.2:
>>>>>   @Priority: which package is this? I guess commons-annoations,
> but where
>>> got this specced?
>>>>   In the common annotation MR
>>>>
>>>>>     Btw, it would be good to have the magic number as float…
>>>>   Float is too complicated ;)
>>> I think int is large enough.
>>>
>>>>>   "If an array-valued or annotation-valued member of an
> interceptor
>>> binding type is not annotated with the Nonbinding annotation,
> non-portable
>>> behavior results."
>>>>>   Wasn't there a solution for the array issue over on our
> CDI EG
>>> list?
>>>>   Pete?
>>> We discussed it, but there are still problems with it IIRC. We can
> always add
>>> this later.
>>>
>>>>>   Chapter 4:
>>>>>   "Method-level target class method interceptors are
> invoked in
>>> addition to any default interceptors and interceptors defined for the
> target
>>> class (and its superclasses)."
>>>>>   Imo this is way too blurry. Not 'in addition' but we
> should
>>> instead define whether 'before' or 'after', isn't?
>>>>   It's in addition - see rules in 5.3 (this section is changing
> places
>>> with 5.4/5.5 as we speak)
>>>
>>> Right the order is elsewhere.
>>>
>>>>>     We should also define the order of subclass/superclass
> interceptors.
>>> For superclass it also depends whether the @InterceptorBinding has
> @Inherited or
>>> not, don't it? Or just apply standard Java rules and let
>>> 'overriding' kick in?
>>>>   Pete said that subclass/superclass order is the same for the CDI
> and EJB
>>> interceptors.
>>>
>>> AFAIK the order is undefined.
>>>
>>>>>   Chapter 5.2:
>>>>>   "If a target class has superclasses, any interceptor
> methods
>>> defined on those superclasses are invoked, most general superclass
> first."
>>>>>   This is true for all but @PreDestroy and @PrePassivate
> interceptors.
>>> Those must not follow ct rules but finalize rules and imo should get
> invoked in
>>> subclass-first order.
>>>>   It's correct. See above ;(
>>>>>   Chapter 5.4:
>>>>>   a.) @Priority should get a float instead of an int. It's
> easier to
>>> squeeze something in that way.
>>>>>   b.) We must clarify the relation between a priority defined
> in
>>> beans.xml and @Priority, or should we do that in the CDI spec?
>>>>   In the CDI spec ;)
>>> Yes. I need to do this.
>>>
>>>>>     What about EJBs then?
>>>>   It said  - does not apply.
>>>>
>>>>>
>>>>>   Well, I only did read through it once and I guess I got a few
> things
>>> wrong. But at least these are a few ideas floating around in my head.
> Hope it
>>> helps.
>>>>   It does. As you can see, it's not an easy task to merge 2
> specs... If
>>> you have any other comments in the next couple of days, please post
> them. The MR
>>> specs are due for the JCP submission on the 7th.
>>>>   Best,
>>>>   -marina
>>>>
>>>>>   LieGrue,
>>>>>   strub
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>   ----- Original Message -----
>>>>>>   From: Pete Muir <pmuir_at_redhat.com>
>>>>>>   To: cdi-dev_at_lists.jboss.org
>>>>>>   Cc:
>>>>>>   Sent: Wednesday, January 30, 2013 8:39 AM
>>>>>>   Subject: [cdi-dev] Fwd: Interceptors spec 1.2 draft is
> available
>>> for review
>>>>>>   FYI - big improvements to the interceptors spec - much
> better for
>>> alignment with
>>>>>>   CDI.
>>>>>>
>>>>>>   I'll update CDI, next, to take account of this.
>>>>>>
>>>>>>   Begin forwarded message:
>>>>>>
>>>>>>>     From: Marina Vatkina
> <marina.vatkina_at_oracle.com>
>>>>>>>     Subject: Interceptors spec 1.2 draft is available
> for review
>>>>>>>     Date: 29 January 2013 22:06:36 GMT
>>>>>>>     To: jsr345-experts_at_ejb-spec.java.net, Pete Muir
>>> <pmuir_at_redhat.com>
>>>>>>>     After a lot of work by Linda, me, and Pete, we have
> the
>>> Interceptors spec
>>>>>>   1.2 draft for review:
>>>>>>
>>>
> http://java.net/projects/interceptors-spec/downloads/download/interceptor-1-2-dr1.pdf
>>>>>>>     What's there:
>>>>>>>     Editorial cleanup and conversion to standard
> template.
>>>>>>>     Assigned chapter numbers to sections and rearranged
> various
>>> sections and
>>>>>>   examples for better flow.
>>>>>>>     Clarified statement regarding transaction context of
> lifecycle
>>> callback
>>>>>>   methods
>>>>>>>     Added a note on a timeout method that is also a
> business
>>> method and
>>>>>>   around-timeout and around-invoke interceptors
>>>>>>>     Added Chapter 1 (Overview)
>>>>>>>     Added Chapter 3, derived from Chapter 9 of the CDI
>>> specification.
>>>>>>>     Removed deployment descriptors definitions (general
> notes
>>> about possibility
>>>>>>   of DDs are there)
>>>>>>>     Added examples with interceptor bindings to common
> sections.
>>>>>>>     Added standard Priority ranges
>>>>>>>
>>>>>>>     What's not there:
>>>>>>>     @AroundConstruct interceptor
>>>>>>>     Notes on "throws Exception" clauses in the
>>> around-xxx method
>>>>>>   signatures
>>>>>>>     Perfect CDI alignment - the text (and fonts) might
> be not
>>> fully aligned.
>>>>>>>     Please review ASAP.
>>>>>>>
>>>>>>>     Pete, please forward to the CDI EG.
>>>>>>>
>>>>>>>     thanks,
>>>>>>>     -marina
>>>>>>   _______________________________________________
>>>>>>   cdi-dev mailing list
>>>>>>   cdi-dev_at_lists.jboss.org
>>>>>>   https://lists.jboss.org/mailman/listinfo/cdi-dev
>>>>>>
>