logging-log4j-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gary Gregory <garydgreg...@gmail.com>
Subject Re: [1/4] logging-log4j2 git commit: Add AutoCloseableLock.
Date Fri, 22 Jul 2016 00:22:53 GMT
Interesting idea! I updated the AutoCloseableLock with this change.
AutoCloseableLock now
implements Lock.

Gary

On Thu, Jul 21, 2016 at 5:14 PM, Matt Sicker <boards@gmail.com> wrote:

> And to distinguish it, why not just give it a signature like:
>
> AutoCloseable autoLock();
>
> try (final AutoCloseable a = lock.autoLock()) {
>   // ...
> }
>
> This way you can inherit from Lock as well.
>
> On 21 July 2016 at 19:12, Matt Sicker <boards@gmail.com> wrote:
>
>> AutoCloseable versus AutoCloseableLock.
>>
>> On 21 July 2016 at 18:19, Gary Gregory <garydgregory@gmail.com> wrote:
>>
>>> On Thu, Jul 21, 2016 at 4:12 PM, Matt Sicker <boards@gmail.com> wrote:
>>>
>>>> Well in that case, I guess it doesn't introduce garbage. I'm back to
>>>> supporting this, then.
>>>>
>>>> Can't you just do:
>>>>
>>>> try (final AutoCloseable a = configLock.lock()) {
>>>>   // ...
>>>> }
>>>>
>>>
>>> That's just the example I gave! Am I missing something? ;-)
>>>
>>> Gary
>>>
>>>>
>>>> On 21 July 2016 at 18:10, Gary Gregory <garydgregory@gmail.com> wrote:
>>>>
>>>>> Hi All,
>>>>>
>>>>> What extra garbage? (Putting aside if this a "good" idea or not.) A
>>>>> local variable here does not create an extra object to be GC'd.
>>>>>
>>>>> For example, in LoggerContext, we have:
>>>>>
>>>>>     private final Lock configLock = new ReentrantLock();
>>>>>     ...
>>>>>
>>>>>     @Override
>>>>>     public void stop() {
>>>>>         ...
>>>>>         configLock.lock();
>>>>>         try {
>>>>>             ...
>>>>>         } finally {
>>>>>             configLock.unlock();
>>>>>         }
>>>>>         ...
>>>>>     }
>>>>>
>>>>>
>>>>> Which I propose to replace with:
>>>>>
>>>>>     private final AutoCloseableLock configLock =
>>>>> AutoCloseableLock.forReentrantLock();
>>>>>     ...
>>>>>
>>>>>     @Override
>>>>>     public void stop() {
>>>>>         ...
>>>>>         try (final AutoCloseableLock l = configLock.lock()) {
>>>>>            ...
>>>>>         }
>>>>>         ...
>>>>>     }
>>>>>
>>>>> Yes, AutoCloseableLock is an extra object on top of the Lock itself,
>>>>> I'll give you that of course.
>>>>>
>>>>> The "l" lvar is what the Java syntax requires but there is no extra
>>>>> object created by configLock.lock(), which returns "this".
>>>>>
>>>>> Gary
>>>>>
>>>>>
>>>>> On Thu, Jul 21, 2016 at 3:12 PM, Matt Sicker <boards@gmail.com>
wrote:
>>>>>
>>>>>> Yeah, adding garbage does seem like a bad idea in this case.
>>>>>>
>>>>>> On 21 July 2016 at 16:30, Ralph Goers <ralph.goers@dslextreme.com>
>>>>>> wrote:
>>>>>>
>>>>>>> The more I think about this the more I dislike it. This requires
>>>>>>> that a temporary variable be created for no other reason than
to satisfy
>>>>>>> the compiler. It defeats the work Remko has been doing to make
the code
>>>>>>> garbage free as it is explicitly creating objects it doesn’t
even use.
>>>>>>>
>>>>>>> Ralph
>>>>>>>
>>>>>>> On Jul 21, 2016, at 2:12 PM, Gary Gregory <garydgregory@gmail.com>
>>>>>>> wrote:
>>>>>>>
>>>>>>> Hi Matt,
>>>>>>>
>>>>>>> AutoCloseableLock cannot implement Lock because lock() is void
and
>>>>>>> I count on AutoCloseableLock#lock() returning "this" for the
>>>>>>> pattern:
>>>>>>>
>>>>>>>         try (final AutoCloseableLock l = LOCK.lock()) {
>>>>>>>             return MAP.containsKey(name);
>>>>>>>         }
>>>>>>>
>>>>>>> I could rename lock() to doLock() and a void lock() but that
seems a
>>>>>>> bit confusing to have both methods.
>>>>>>>
>>>>>>> This is in the branch AutoCloseableLock which I'd like to merge.
>>>>>>>
>>>>>>> Thoughts?
>>>>>>>
>>>>>>> Gary
>>>>>>>
>>>>>>> On Fri, Jun 24, 2016 at 7:42 AM, Matt Sicker <boards@gmail.com>
>>>>>>> wrote:
>>>>>>>
>>>>>>>> I kinda imagined AutoCloseableLock to implement both AutoCloseable
>>>>>>>> and Lock.
>>>>>>>>
>>>>>>>> ---------- Forwarded message ----------
>>>>>>>> From: <ggregory@apache.org>
>>>>>>>> Date: 24 June 2016 at 03:50
>>>>>>>> Subject: [1/4] logging-log4j2 git commit: Add AutoCloseableLock.
>>>>>>>> To: commits@logging.apache.org
>>>>>>>>
>>>>>>>>
>>>>>>>> Repository: logging-log4j2
>>>>>>>> Updated Branches:
>>>>>>>>   refs/heads/AutoCloseableLock [created] 72d9978c6
>>>>>>>>
>>>>>>>>
>>>>>>>> Add AutoCloseableLock.
>>>>>>>>
>>>>>>>> Project: http://git-wip-us.apache.org/repos/asf/logging-log4j2/repo
>>>>>>>> Commit:
>>>>>>>> http://git-wip-us.apache.org/repos/asf/logging-log4j2/commit/40efa80a
>>>>>>>> Tree:
>>>>>>>> http://git-wip-us.apache.org/repos/asf/logging-log4j2/tree/40efa80a
>>>>>>>> Diff:
>>>>>>>> http://git-wip-us.apache.org/repos/asf/logging-log4j2/diff/40efa80a
>>>>>>>>
>>>>>>>> Branch: refs/heads/AutoCloseableLock
>>>>>>>> Commit: 40efa80a1a9745d7f9162b4f7ce96a7571a1c336
>>>>>>>> Parents: bc296c5
>>>>>>>> Author: Gary Gregory <ggregory@apache.org>
>>>>>>>> Authored: Thu Jun 23 21:59:02 2016 -0700
>>>>>>>> Committer: Gary Gregory <ggregory@apache.org>
>>>>>>>> Committed: Thu Jun 23 21:59:02 2016 -0700
>>>>>>>>
>>>>>>>>
>>>>>>>> ----------------------------------------------------------------------
>>>>>>>>  .../logging/log4j/util/AutoCloseableLock.java   | 44
>>>>>>>> ++++++++++++++++++++
>>>>>>>>  1 file changed, 44 insertions(+)
>>>>>>>>
>>>>>>>> ----------------------------------------------------------------------
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/40efa80a/log4j-api/src/main/java/org/apache/logging/log4j/util/AutoCloseableLock.java
>>>>>>>>
>>>>>>>> ----------------------------------------------------------------------
>>>>>>>> diff --git
>>>>>>>> a/log4j-api/src/main/java/org/apache/logging/log4j/util/AutoCloseableLock.java
>>>>>>>> b/log4j-api/src/main/java/org/apache/logging/log4j/util/AutoCloseableLock.java
>>>>>>>> new file mode 100644
>>>>>>>> index 0000000..65aa581
>>>>>>>> --- /dev/null
>>>>>>>> +++
>>>>>>>> b/log4j-api/src/main/java/org/apache/logging/log4j/util/AutoCloseableLock.java
>>>>>>>> @@ -0,0 +1,44 @@
>>>>>>>> +/*
>>>>>>>> + * Licensed to the Apache Software Foundation (ASF) under
one or
>>>>>>>> more
>>>>>>>> + * contributor license agreements. See the NOTICE file distributed
>>>>>>>> with
>>>>>>>> + * this work for additional information regarding copyright
>>>>>>>> ownership.
>>>>>>>> + * The ASF licenses this file to You under the Apache license,
>>>>>>>> Version 2.0
>>>>>>>> + * (the "License"); you may not use this file except in
compliance
>>>>>>>> with
>>>>>>>> + * the License. You may obtain a copy of the License at
>>>>>>>> + *
>>>>>>>> + *      http://www.apache.org/licenses/LICENSE-2.0
>>>>>>>> + *
>>>>>>>> + * Unless required by applicable law or agreed to in writing,
>>>>>>>> software
>>>>>>>> + * distributed under the License is distributed on an "AS
IS"
>>>>>>>> BASIS,
>>>>>>>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
express or
>>>>>>>> implied.
>>>>>>>> + * See the license for the specific language governing permissions
>>>>>>>> and
>>>>>>>> + * limitations under the license.
>>>>>>>> + */
>>>>>>>> +package org.apache.logging.log4j.util;
>>>>>>>> +
>>>>>>>> +import java.util.Objects;
>>>>>>>> +import java.util.concurrent.locks.Lock;
>>>>>>>> +
>>>>>>>> +public class AutoCloseableLock implements AutoCloseable
{
>>>>>>>> +    public static AutoCloseableLock lock(final Lock lock)
{
>>>>>>>> +        Objects.requireNonNull(lock, "lock");
>>>>>>>> +        lock.lock();
>>>>>>>> +        return new AutoCloseableLock(lock);
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    private final Lock lock;
>>>>>>>> +
>>>>>>>> +    public AutoCloseableLock(final Lock lock) {
>>>>>>>> +        this.lock = lock;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    @Override
>>>>>>>> +    public void close() {
>>>>>>>> +        this.lock.unlock();
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    public AutoCloseableLock lock() {
>>>>>>>> +        this.lock.lock();
>>>>>>>> +        return this;
>>>>>>>> +    }
>>>>>>>> +}
>>>>>>>> \ No newline at end of file
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> Matt Sicker <boards@gmail.com>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> E-Mail: garydgregory@gmail.com | ggregory@apache.org
>>>>>>> Java Persistence with Hibernate, Second Edition
>>>>>>> <http://www.manning.com/bauer3/>
>>>>>>> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
>>>>>>> Spring Batch in Action <http://www.manning.com/templier/>
>>>>>>> Blog: http://garygregory.wordpress.com
>>>>>>> Home: http://garygregory.com/
>>>>>>> Tweet! http://twitter.com/GaryGregory
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Matt Sicker <boards@gmail.com>
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> E-Mail: garydgregory@gmail.com | ggregory@apache.org
>>>>> Java Persistence with Hibernate, Second Edition
>>>>> <http://www.manning.com/bauer3/>
>>>>> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
>>>>> Spring Batch in Action <http://www.manning.com/templier/>
>>>>> Blog: http://garygregory.wordpress.com
>>>>> Home: http://garygregory.com/
>>>>> Tweet! http://twitter.com/GaryGregory
>>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> Matt Sicker <boards@gmail.com>
>>>>
>>>
>>>
>>>
>>> --
>>> E-Mail: garydgregory@gmail.com | ggregory@apache.org
>>> Java Persistence with Hibernate, Second Edition
>>> <http://www.manning.com/bauer3/>
>>> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
>>> Spring Batch in Action <http://www.manning.com/templier/>
>>> Blog: http://garygregory.wordpress.com
>>> Home: http://garygregory.com/
>>> Tweet! http://twitter.com/GaryGregory
>>>
>>
>>
>>
>> --
>> Matt Sicker <boards@gmail.com>
>>
>
>
>
> --
> Matt Sicker <boards@gmail.com>
>



-- 
E-Mail: garydgregory@gmail.com | ggregory@apache.org
Java Persistence with Hibernate, Second Edition
<http://www.manning.com/bauer3/>
JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
Spring Batch in Action <http://www.manning.com/templier/>
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory

Mime
View raw message