Re: git commit: DELTASPIKE-424 taking into account EntityManagerResolver with a normal scope

2013-10-11 Thread Karl Kildén 
Hello!

I have some trouble understanding why it's bad to destroy the instance with
that example. What about this example, does it make sense?

DependentProviderContextControl ctxControl =
BeanProvider.getDependent(ContextControl.class);

ctxControl.get().startContext(ApplicationScoped.class);
// Do something useful in for example a Quartz Job

ctxControl.destroy();

cheers




On 11 October 2013 10:15, Mark Struberg strub...@yahoo.de wrote:


 If you don't destroy it you'll likely leak.
 Yes, fully agree. But the way we do it is still wrong. IF it is a
 @Dependent scoped instance, then we must store the
 DependentProviderEntityManager somewhere and only invoke it's destroy()
 method once we really need.

 For NormalScoped beans you are relieved of this burden, but for @Dependent
 ones you need to handle it manually. The DependentProvider just helps to
 easily store the CreationalContext, the BeanT and the instance for
 convenience reasons.


 LieGrue,
 strub


 
  From: Romain Manni-Bucau rmannibu...@gmail.com
 To: dev@deltaspike.apache.org dev@deltaspike.apache.org; Mark
 Struberg strub...@yahoo.de
 Sent: Thursday, 10 October 2013, 9:07
 Subject: Re: git commit: DELTASPIKE-424 taking into account
 EntityManagerResolver with a normal scope
 
 
 If you don't destroy it you'll likely leak.
 
 And once again you are in your case where you handle the emf yourself. In
 other cases @Dependent should work fine.
 
 That said nothing again preventing using @Dependent so just do it If it
 solves the issue. Normal scopes were the important part for me.
 
 *Romain Manni-Bucau*
 *Twitter: @rmannibucau https://twitter.com/rmannibucau*
 *Blog: **http://rmannibucau.wordpress.com/*
 http://rmannibucau.wordpress.com/
 *LinkedIn: **http://fr.linkedin.com/in/rmannibucau*
 *Github: https://github.com/rmannibucau*
 
 
 
 
 2013/10/10 Mark Struberg strub...@yahoo.de
 
 
  Both works
 
  That's exactly where I disagree. While both 'work' in the sample,
  immediately destroying the instances again after creating them - and
 only
  later returning the java reference to the now 'dead'
 EntityManagerResolver
  - is broken if you will use it in productive scenarios.
 
 
  Either we fix this, or we don't need any special handling. In this case
 I
  suggest to just use DependentProvider.get() for all cases. It has no
 harm
  on NormalScoped instances anyway.
 
  I will guard DependentProvider#destroy to not have any impact on
  @Dependent scoped instances.
 
 
  LieGrue,
  strub
 
 
  
   From: Romain Manni-Bucau rmannibu...@gmail.com
  To: dev@deltaspike.apache.org dev@deltaspike.apache.org; Mark
  Struberg strub...@yahoo.de
  Sent: Thursday, 10 October 2013, 8:33
  Subject: Re: git commit: DELTASPIKE-424 taking into account
  EntityManagerResolver with a normal scope
  
  
  
  Both works Mark depending as you said from where you take your em. We
  just need to be explicit on this behavior. BTW I prefer the normal scope
  usage too.
  
  
  Romain Manni-Bucau
  Twitter: @rmannibucau
  Blog: http://rmannibucau.wordpress.com/
  LinkedIn: http://fr.linkedin.com/in/rmannibucau
  Github: https://github.com/rmannibucau
  
  
  
  
  2013/10/10 Mark Struberg strub...@yahoo.de
  
  Not sure if we really need this flag.
  The most important question to me is _when_ do we trigger the
 destroy()
  method.
  
  The following code doesn't make much sense imo:
  
   final DependentProvider? extends EntityManagerResolver resolver =
  lookupResolver(emrc);
   result = resolver.get().resolveEntityManager();
   resolver.destroy();
  
  
  The DependentProvider is intended to store it's instances somewhere to
  be able to properly destroy the created @Dependent instance once you
 don't
  need it anymore. Invoking destroy() immediately but returning the
 created
  contextual instance makes no sense imo. Imagine what happens if you
 would
  close the EntityManagerFactory in a @PreDestroy method.
  
  If there is no clean way to clean up the instance again, then we
 should
  only rely on NormalScoped instances and remove the handling (and get the
  warnings again, because they make sense).
  
  LieGrue,
  strub
  
  
  
  
  
  
  
  - Original Message -
   From: rmannibu...@apache.org rmannibu...@apache.org
   To: comm...@deltaspike.apache.org
   Cc:
   Sent: Wednesday, 9 October 2013, 16:46
   Subject: git commit: DELTASPIKE-424 taking into account
  EntityManagerResolver with a normal scope
  
   Updated Branches:
 refs/heads/master bdc9cdce6 - e8148110e
  
  
   DELTASPIKE-424 taking into account EntityManagerResolver with a
 normal
  scope
  
  
   Project: http://git-wip-us.apache.org/repos/asf/deltaspike/repo
   Commit:
  http://git-wip-us.apache.org/repos/asf/deltaspike/commit/e8148110
   Tree:
 http://git-wip-us.apache.org/repos/asf/deltaspike/tree/e8148110
   Diff:
 http://git-wip-us.apache.org/repos/asf/deltaspike/diff

Re: git commit: DELTASPIKE-424 taking into account EntityManagerResolver with a normal scope

2013-10-10 Thread Mark Struberg 

Both works

That's exactly where I disagree. While both 'work' in the sample, immediately 
destroying the instances again after creating them - and only later returning 
the java reference to the now 'dead' EntityManagerResolver - is broken if you 
will use it in productive scenarios. 


Either we fix this, or we don't need any special handling. In this case I 
suggest to just use DependentProvider.get() for all cases. It has no harm on 
NormalScoped instances anyway.

I will guard DependentProvider#destroy to not have any impact on @Dependent 
scoped instances.


LieGrue,
strub



 From: Romain Manni-Bucau rmannibu...@gmail.com
To: dev@deltaspike.apache.org dev@deltaspike.apache.org; Mark Struberg 
strub...@yahoo.de 
Sent: Thursday, 10 October 2013, 8:33
Subject: Re: git commit: DELTASPIKE-424 taking into account 
EntityManagerResolver with a normal scope
 


Both works Mark depending as you said from where you take your em. We just 
need to be explicit on this behavior. BTW I prefer the normal scope usage too.


Romain Manni-Bucau
Twitter: @rmannibucau
Blog: http://rmannibucau.wordpress.com/
LinkedIn: http://fr.linkedin.com/in/rmannibucau
Github: https://github.com/rmannibucau




2013/10/10 Mark Struberg strub...@yahoo.de

Not sure if we really need this flag.
The most important question to me is _when_ do we trigger the destroy() 
method.

The following code doesn't make much sense imo:

 final DependentProvider? extends EntityManagerResolver resolver = 
 lookupResolver(emrc);
 result = resolver.get().resolveEntityManager();
 resolver.destroy();


The DependentProvider is intended to store it's instances somewhere to be 
able to properly destroy the created @Dependent instance once you don't need 
it anymore. Invoking destroy() immediately but returning the created 
contextual instance makes no sense imo. Imagine what happens if you would 
close the EntityManagerFactory in a @PreDestroy method.

If there is no clean way to clean up the instance again, then we should only 
rely on NormalScoped instances and remove the handling (and get the warnings 
again, because they make sense).

LieGrue,
strub







- Original Message -
 From: rmannibu...@apache.org rmannibu...@apache.org
 To: comm...@deltaspike.apache.org
 Cc:
 Sent: Wednesday, 9 October 2013, 16:46
 Subject: git commit: DELTASPIKE-424 taking into account 
 EntityManagerResolver with a normal scope

 Updated Branches:
   refs/heads/master bdc9cdce6 - e8148110e


 DELTASPIKE-424 taking into account EntityManagerResolver with a normal scope


 Project: http://git-wip-us.apache.org/repos/asf/deltaspike/repo
 Commit: http://git-wip-us.apache.org/repos/asf/deltaspike/commit/e8148110
 Tree: http://git-wip-us.apache.org/repos/asf/deltaspike/tree/e8148110
 Diff: http://git-wip-us.apache.org/repos/asf/deltaspike/diff/e8148110

 Branch: refs/heads/master
 Commit: e8148110ea2458fa2244a439583da0f2adddb482
 Parents: bdc9cdc
 Author: Romain Manni-Bucau rmannibu...@apache.org
 Authored: Wed Oct 9 16:46:06 2013 +0200
 Committer: Romain Manni-Bucau rmannibu...@apache.org
 Committed: Wed Oct 9 16:46:06 2013 +0200

 --
 .../data/impl/RepositoryExtension.java          |  2 +-
 .../data/impl/handler/EntityManagerLookup.java  | 20 +++--
 .../data/impl/meta/RepositoryComponent.java     | 23 +++-
 .../data/impl/meta/RepositoryComponents.java    | 16 --
 .../data/impl/builder/part/QueryRootTest.java   |  5 ++---
 5 files changed, 47 insertions(+), 19 deletions(-)
 --


 http://git-wip-us.apache.org/repos/asf/deltaspike/blob/e8148110/deltaspike/modules/data/impl/src/main/java/org/apache/deltaspike/data/impl/RepositoryExtension.java
 --
 diff --git
 a/deltaspike/modules/data/impl/src/main/java/org/apache/deltaspike/data/impl/RepositoryExtension.java
 b/deltaspike/modules/data/impl/src/main/java/org/apache/deltaspike/data/impl/RepositoryExtension.java
 index 076393b..8ba0dca 100755
 ---
 a/deltaspike/modules/data/impl/src/main/java/org/apache/deltaspike/data/impl/RepositoryExtension.java
 +++
 b/deltaspike/modules/data/impl/src/main/java/org/apache/deltaspike/data/impl/RepositoryExtension.java
 @@ -72,7 +72,7 @@ public class RepositoryExtension implements Extension
              {
                  log.log(Level.FINER, getHandlerClass: Repository
 annotation detected on {0},
                          event.getAnnotatedType());
 -                RepositoryComponentsFactory.instance().add(repoClass);
 +                RepositoryComponentsFactory.instance().add(repoClass,
 beanManager);
              }
              catch (RepositoryDefinitionException e)
              {

 http://git-wip-us.apache.org/repos/asf/deltaspike/blob/e8148110/deltaspike/modules/data/impl/src/main/java/org/apache