I've made Service extend from Closeable

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

I've made Service extend from Closeable

Zoran Regvart-2
Hi,
just giving the discussion on the PR#1537[1], CAMEL-11011[2] more
visibility here if anyone would like to comment.
The downside I found was that the IDE issues a warning about
'Potential resource leak'. I don't know if this would be reason enough
to revert this?

zoran

[1] https://github.com/apache/camel/pull/1537
[2] https://issues.apache.org/jira/browse/CAMEL-11011
--
Zoran Regvart
Reply | Threaded
Open this post in threaded view
|

Re: I've made Service extend from Closeable

Claus Ibsen-2
Hi

Is there an example you can point we can open in our IDEs to see ?


On Wed, Mar 15, 2017 at 9:49 AM, Zoran Regvart <[hidden email]> wrote:

> Hi,
> just giving the discussion on the PR#1537[1], CAMEL-11011[2] more
> visibility here if anyone would like to comment.
> The downside I found was that the IDE issues a warning about
> 'Potential resource leak'. I don't know if this would be reason enough
> to revert this?
>
> zoran
>
> [1] https://github.com/apache/camel/pull/1537
> [2] https://issues.apache.org/jira/browse/CAMEL-11011
> --
> Zoran Regvart



--
Claus Ibsen
-----------------
http://davsclaus.com @davsclaus
Camel in Action 2: https://www.manning.com/ibsen2
Reply | Threaded
Open this post in threaded view
|

Re: I've made Service extend from Closeable

Zoran Regvart-2
Hi,
for instance in BeanLookupUsingJndiRegistryIssueTest.java:34[1], the
warrning I get in Eclipse is:

"Resource leak: 'camel' is never closed"

this is because the resource leak detection code in IDE is looking for
close(), but we have stop()

zoran

[1] https://github.com/apache/camel/blob/master/camel-core/src/test/java/org/apache/camel/component/bean/BeanLookupUsingJndiRegistryIssueTest.java#L34

On Wed, Mar 15, 2017 at 10:01 AM, Claus Ibsen <[hidden email]> wrote:

> Hi
>
> Is there an example you can point we can open in our IDEs to see ?
>
>
> On Wed, Mar 15, 2017 at 9:49 AM, Zoran Regvart <[hidden email]> wrote:
>> Hi,
>> just giving the discussion on the PR#1537[1], CAMEL-11011[2] more
>> visibility here if anyone would like to comment.
>> The downside I found was that the IDE issues a warning about
>> 'Potential resource leak'. I don't know if this would be reason enough
>> to revert this?
>>
>> zoran
>>
>> [1] https://github.com/apache/camel/pull/1537
>> [2] https://issues.apache.org/jira/browse/CAMEL-11011
>> --
>> Zoran Regvart
>
>
>
> --
> Claus Ibsen
> -----------------
> http://davsclaus.com @davsclaus
> Camel in Action 2: https://www.manning.com/ibsen2



--
Zoran Regvart
Reply | Threaded
Open this post in threaded view
|

Re: I've made Service extend from Closeable

Claus Ibsen-2
Hi

Okay se we do not see this in IDEA - its after all a smarter editor ;)

On Wed, Mar 15, 2017 at 10:26 AM, Zoran Regvart <[hidden email]> wrote:

> Hi,
> for instance in BeanLookupUsingJndiRegistryIssueTest.java:34[1], the
> warrning I get in Eclipse is:
>
> "Resource leak: 'camel' is never closed"
>
> this is because the resource leak detection code in IDE is looking for
> close(), but we have stop()
>
> zoran
>
> [1] https://github.com/apache/camel/blob/master/camel-core/src/test/java/org/apache/camel/component/bean/BeanLookupUsingJndiRegistryIssueTest.java#L34
>
> On Wed, Mar 15, 2017 at 10:01 AM, Claus Ibsen <[hidden email]> wrote:
>> Hi
>>
>> Is there an example you can point we can open in our IDEs to see ?
>>
>>
>> On Wed, Mar 15, 2017 at 9:49 AM, Zoran Regvart <[hidden email]> wrote:
>>> Hi,
>>> just giving the discussion on the PR#1537[1], CAMEL-11011[2] more
>>> visibility here if anyone would like to comment.
>>> The downside I found was that the IDE issues a warning about
>>> 'Potential resource leak'. I don't know if this would be reason enough
>>> to revert this?
>>>
>>> zoran
>>>
>>> [1] https://github.com/apache/camel/pull/1537
>>> [2] https://issues.apache.org/jira/browse/CAMEL-11011
>>> --
>>> Zoran Regvart
>>
>>
>>
>> --
>> Claus Ibsen
>> -----------------
>> http://davsclaus.com @davsclaus
>> Camel in Action 2: https://www.manning.com/ibsen2
>
>
>
> --
> Zoran Regvart



--
Claus Ibsen
-----------------
http://davsclaus.com @davsclaus
Camel in Action 2: https://www.manning.com/ibsen2
Reply | Threaded
Open this post in threaded view
|

Re: I've made Service extend from Closeable

Zoran Regvart-2
Yeah,
I'm only worried that this would introduce confusion amongst users,
and once we release a version of Camel with this it's forever there.

zoran

On Thu, Mar 16, 2017 at 3:13 PM, Claus Ibsen <[hidden email]> wrote:

> Hi
>
> Okay se we do not see this in IDEA - its after all a smarter editor ;)
>
> On Wed, Mar 15, 2017 at 10:26 AM, Zoran Regvart <[hidden email]> wrote:
>> Hi,
>> for instance in BeanLookupUsingJndiRegistryIssueTest.java:34[1], the
>> warrning I get in Eclipse is:
>>
>> "Resource leak: 'camel' is never closed"
>>
>> this is because the resource leak detection code in IDE is looking for
>> close(), but we have stop()
>>
>> zoran
>>
>> [1] https://github.com/apache/camel/blob/master/camel-core/src/test/java/org/apache/camel/component/bean/BeanLookupUsingJndiRegistryIssueTest.java#L34
>>
>> On Wed, Mar 15, 2017 at 10:01 AM, Claus Ibsen <[hidden email]> wrote:
>>> Hi
>>>
>>> Is there an example you can point we can open in our IDEs to see ?
>>>
>>>
>>> On Wed, Mar 15, 2017 at 9:49 AM, Zoran Regvart <[hidden email]> wrote:
>>>> Hi,
>>>> just giving the discussion on the PR#1537[1], CAMEL-11011[2] more
>>>> visibility here if anyone would like to comment.
>>>> The downside I found was that the IDE issues a warning about
>>>> 'Potential resource leak'. I don't know if this would be reason enough
>>>> to revert this?
>>>>
>>>> zoran
>>>>
>>>> [1] https://github.com/apache/camel/pull/1537
>>>> [2] https://issues.apache.org/jira/browse/CAMEL-11011
>>>> --
>>>> Zoran Regvart
>>>
>>>
>>>
>>> --
>>> Claus Ibsen
>>> -----------------
>>> http://davsclaus.com @davsclaus
>>> Camel in Action 2: https://www.manning.com/ibsen2
>>
>>
>>
>> --
>> Zoran Regvart
>
>
>
> --
> Claus Ibsen
> -----------------
> http://davsclaus.com @davsclaus
> Camel in Action 2: https://www.manning.com/ibsen2



--
Zoran Regvart
Reply | Threaded
Open this post in threaded view
|

Re: I've made Service extend from Closeable

Claus Ibsen-2
On Thu, Mar 16, 2017 at 11:51 PM, Zoran Regvart <[hidden email]> wrote:
> Yeah,
> I'm only worried that this would introduce confusion amongst users,
> and once we release a version of Camel with this it's forever there.
>

Yeah we could revert it.

Or what if you add closable on ServiceSupport which they all extend,
would it work with try with resources still, and then you can
implement the close method directly on ServiceSupport which maybe
would not cause Eclipse to show that problem?

If not we can log a ticket about an API change for Camel 3.0. And look
at this then.

> zoran
>
> On Thu, Mar 16, 2017 at 3:13 PM, Claus Ibsen <[hidden email]> wrote:
>> Hi
>>
>> Okay se we do not see this in IDEA - its after all a smarter editor ;)
>>
>> On Wed, Mar 15, 2017 at 10:26 AM, Zoran Regvart <[hidden email]> wrote:
>>> Hi,
>>> for instance in BeanLookupUsingJndiRegistryIssueTest.java:34[1], the
>>> warrning I get in Eclipse is:
>>>
>>> "Resource leak: 'camel' is never closed"
>>>
>>> this is because the resource leak detection code in IDE is looking for
>>> close(), but we have stop()
>>>
>>> zoran
>>>
>>> [1] https://github.com/apache/camel/blob/master/camel-core/src/test/java/org/apache/camel/component/bean/BeanLookupUsingJndiRegistryIssueTest.java#L34
>>>
>>> On Wed, Mar 15, 2017 at 10:01 AM, Claus Ibsen <[hidden email]> wrote:
>>>> Hi
>>>>
>>>> Is there an example you can point we can open in our IDEs to see ?
>>>>
>>>>
>>>> On Wed, Mar 15, 2017 at 9:49 AM, Zoran Regvart <[hidden email]> wrote:
>>>>> Hi,
>>>>> just giving the discussion on the PR#1537[1], CAMEL-11011[2] more
>>>>> visibility here if anyone would like to comment.
>>>>> The downside I found was that the IDE issues a warning about
>>>>> 'Potential resource leak'. I don't know if this would be reason enough
>>>>> to revert this?
>>>>>
>>>>> zoran
>>>>>
>>>>> [1] https://github.com/apache/camel/pull/1537
>>>>> [2] https://issues.apache.org/jira/browse/CAMEL-11011
>>>>> --
>>>>> Zoran Regvart
>>>>
>>>>
>>>>
>>>> --
>>>> Claus Ibsen
>>>> -----------------
>>>> http://davsclaus.com @davsclaus
>>>> Camel in Action 2: https://www.manning.com/ibsen2
>>>
>>>
>>>
>>> --
>>> Zoran Regvart
>>
>>
>>
>> --
>> Claus Ibsen
>> -----------------
>> http://davsclaus.com @davsclaus
>> Camel in Action 2: https://www.manning.com/ibsen2
>
>
>
> --
> Zoran Regvart



--
Claus Ibsen
-----------------
http://davsclaus.com @davsclaus
Camel in Action 2: https://www.manning.com/ibsen2
Reply | Threaded
Open this post in threaded view
|

Re: I've made Service extend from Closeable

Zoran Regvart-2
Hi Claus,

On Fri, Mar 17, 2017 at 9:09 AM, Claus Ibsen <[hidden email]> wrote:
> On Thu, Mar 16, 2017 at 11:51 PM, Zoran Regvart <[hidden email]> wrote:
>> Yeah,
>> I'm only worried that this would introduce confusion amongst users,
>> and once we release a version of Camel with this it's forever there.
>>
>
> Yeah we could revert it.

I'm leaning on doing that, and as a side note I should have based that
on java.lang.AutoCloseable not on java.io.Closeable, that would make
more sense

> Or what if you add closable on ServiceSupport which they all extend,
> would it work with try with resources still, and then you can
> implement the close method directly on ServiceSupport which maybe
> would not cause Eclipse to show that problem?

doesn't help, I think it tracks if the call to close() is from user
code, which is makes sense if it doesn't look at the call graph, so if
I move the logic in stop() into close() and make stop() delegate to
close() I get the same warning, and from what I can read in the
Eclipse docs[1] this is not the case today

> If not we can log a ticket about an API change for Camel 3.0. And look
> at this then.

perhaps for 3.0 would make sense to switch close() and stop(), or
perhaps in that time tools will look at the call graph

I wonder what other tools like Sonar think about this, I'll try to check that...

zoran

[1] http://help.eclipse.org/neon/index.jsp?topic=%2Forg.eclipse.jdt.doc.user%2Ftasks%2Ftask-avoiding_resource_leaks.htm
--
Zoran Regvart
Reply | Threaded
Open this post in threaded view
|

Re: I've made Service extend from Closeable

Zoran Regvart-2