New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CDI-589 Add Instance.release() method #286

Closed
wants to merge 1 commit into
base: 2.0-EDR2
from

Conversation

Projects
None yet
4 participants
@mkouba
Contributor

mkouba commented Mar 21, 2016

No description provided.

/**
* <p>
* Instructs the container to destroy the instance. The instance passed should be a dependent scoped bean instance

This comment has been minimized.

@manovotn

manovotn Mar 21, 2016

Contributor

should be sounds weird here.
I suppose that if it isn't a dependent scoped bean, you just ignore it.
If that is the case, I would rather say ... instance passed has to be a dependent ....

@manovotn

manovotn Mar 21, 2016

Contributor

should be sounds weird here.
I suppose that if it isn't a dependent scoped bean, you just ignore it.
If that is the case, I would rather say ... instance passed has to be a dependent ....

This comment has been minimized.

@mkouba

mkouba Mar 21, 2016

Contributor

It's intentional. If we say has to be dependent it should probably fail if you pass any other instance (e.g. throw IAE). And we would like to allow users to pass anything and only destroy dependent bean instances. But I'm not native speaker so my interpretation might be wrong ;-)

@mkouba

mkouba Mar 21, 2016

Contributor

It's intentional. If we say has to be dependent it should probably fail if you pass any other instance (e.g. throw IAE). And we would like to allow users to pass anything and only destroy dependent bean instances. But I'm not native speaker so my interpretation might be wrong ;-)

This comment has been minimized.

@manovotn

manovotn Mar 21, 2016

Contributor

Hmm, I see. In that case it makes sense.
Although we might want to name this method so that it indicates the attempt to destroy the instance, wdyt? Something like tryToRelease? Just to draw some clear line between destroy method and this one from user point of view.

@manovotn

manovotn Mar 21, 2016

Contributor

Hmm, I see. In that case it makes sense.
Although we might want to name this method so that it indicates the attempt to destroy the instance, wdyt? Something like tryToRelease? Just to draw some clear line between destroy method and this one from user point of view.

@antoinesd

This comment has been minimized.

Show comment
Hide comment
@antoinesd

antoinesd Mar 21, 2016

Member

I don't know how to put it, but I think we should find a way to make clear why release was introduced when destroy existed.

Member

antoinesd commented Mar 21, 2016

I don't know how to put it, but I think we should find a way to make clear why release was introduced when destroy existed.

@mkouba

This comment has been minimized.

Show comment
Hide comment
@mkouba

mkouba Mar 21, 2016

Contributor

Well, we could mention the difference and describe the use case in javadoc/spec text. Or we could introduce destroy(T instance, boolean ignoreClientProxies) instead. But I like release() more.

Contributor

mkouba commented Mar 21, 2016

Well, we could mention the difference and describe the use case in javadoc/spec text. Or we could introduce destroy(T instance, boolean ignoreClientProxies) instead. But I like release() more.

@mkouba mkouba referenced this pull request May 4, 2016

Closed

Cdi 457 #289

@johnament

This comment has been minimized.

Show comment
Hide comment
@johnament

johnament May 4, 2016

Member

I don't see why we need a release method when we already have a destroy method, when both seem to do the same thing.

Member

johnament commented May 4, 2016

I don't see why we need a release method when we already have a destroy method, when both seem to do the same thing.

@mkouba

This comment has been minimized.

Show comment
Hide comment
@mkouba

mkouba May 4, 2016

Contributor

@johnament See also https://issues.jboss.org/browse/CDI-515 and #273

Instance.destroy() always destroys the bean instance which is not always desirable for normal scopes.

Contributor

mkouba commented May 4, 2016

@johnament See also https://issues.jboss.org/browse/CDI-515 and #273

Instance.destroy() always destroys the bean instance which is not always desirable for normal scopes.

@johnament

This comment has been minimized.

Show comment
Hide comment
@johnament

johnament May 4, 2016

Member

I would be more in favor of fixing the spec to allow you to destroy dependent instances that this instance didn't create.

Member

johnament commented May 4, 2016

I would be more in favor of fixing the spec to allow you to destroy dependent instances that this instance didn't create.

@mkouba

This comment has been minimized.

Show comment
Hide comment
@mkouba

mkouba May 4, 2016

Contributor

to destroy dependent instances that this instance didn't create...

I believe this is not possible. If you don't have the CreationalContext used during dependent instance creation you can't do that.

Contributor

mkouba commented May 4, 2016

to destroy dependent instances that this instance didn't create...

I believe this is not possible. If you don't have the CreationalContext used during dependent instance creation you can't do that.

@mkouba

This comment has been minimized.

Show comment
Hide comment
@mkouba

mkouba May 26, 2016

Contributor

This is what we're working on in Weld:
http://weld.cdi-spec.org/news/2016/05/18/enhanced-instance/

Contributor

mkouba commented May 26, 2016

This is what we're working on in Weld:
http://weld.cdi-spec.org/news/2016/05/18/enhanced-instance/

@antoinesd

This comment has been minimized.

Show comment
Hide comment
@antoinesd

antoinesd Jul 26, 2016

Member

@mkouba re-reading the whole use case and how it was addressed in Weld I wonder why you didn't propose to standardize WeldInstance ?
For me, it adds more sepration between simple/advanced usage and we gain the access to bean metadata that was lost when passing from CDI-515 to CDI-589.

Member

antoinesd commented Jul 26, 2016

@mkouba re-reading the whole use case and how it was addressed in Weld I wonder why you didn't propose to standardize WeldInstance ?
For me, it adds more sepration between simple/advanced usage and we gain the access to bean metadata that was lost when passing from CDI-515 to CDI-589.

@mkouba

This comment has been minimized.

Show comment
Hide comment
@mkouba

mkouba Jul 26, 2016

Contributor

I had a feeling that adding anything with Bean<?> on Instance is not desired. Anyway, we can discuss this at F2F - we plan to release 2.4.CR1 in early October.

Contributor

mkouba commented Jul 26, 2016

I had a feeling that adding anything with Bean<?> on Instance is not desired. Anyway, we can discuss this at F2F - we plan to release 2.4.CR1 in early October.

@mkouba

This comment has been minimized.

Show comment
Hide comment
@mkouba
Contributor

mkouba commented Jul 26, 2016

@antoinesd

This comment has been minimized.

Show comment
Hide comment
@antoinesd

antoinesd Jul 26, 2016

Member

I add it to our Agenda

Member

antoinesd commented Jul 26, 2016

I add it to our Agenda

@johnament

This comment has been minimized.

Show comment
Hide comment
@johnament

johnament Jul 26, 2016

Member

I would go with the handler approach defined by Weld. This PR looks bad IMHO. Can we close it?

Member

johnament commented Jul 26, 2016

I would go with the handler approach defined by Weld. This PR looks bad IMHO. Can we close it?

@antoinesd

This comment has been minimized.

Show comment
Hide comment
@antoinesd

antoinesd Jul 26, 2016

Member

I agree @johnament. It feels half mesure, where WeldInstance contains all the useful feature. Do you agree to close it @mkouba and go for WeldInstance standardization?

Member

antoinesd commented Jul 26, 2016

I agree @johnament. It feels half mesure, where WeldInstance contains all the useful feature. Do you agree to close it @mkouba and go for WeldInstance standardization?

@mkouba

This comment has been minimized.

Show comment
Hide comment
@mkouba

mkouba Jul 26, 2016

Contributor

Sure, close the PR.

Contributor

mkouba commented Jul 26, 2016

Sure, close the PR.

@mkouba mkouba closed this Jul 26, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment