You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "garydgregory (via GitHub)" <gi...@apache.org> on 2023/10/16 19:17:36 UTC
[PR] Use LazyInitializer without subclassing. [commons-lang]
garydgregory opened a new pull request, #1123:
URL: https://github.com/apache/commons-lang/pull/1123
- Allow a Supplier for initialized object
- Allow a Consumer to close the managed object
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Use LazyInitializer without subclassing. [commons-lang]
Posted by "benjamin-confino (via GitHub)" <gi...@apache.org>.
benjamin-confino commented on PR #1123:
URL: https://github.com/apache/commons-lang/pull/1123#issuecomment-1766388011
The PR is here: https://github.com/garydgregory/commons-lang/pull/1
Regarding `get`, `initialize` and `close`. I think it makes sense to use one shared type. `ConcurrentException` is a wrapper around the real exception so its reasonable that even if `get`, `initialize` and `close` return three different exceptions they all use the same wrapper type.
(I didn't remove the recast to IllegalStateException because I wanted to ask why it was there first?)
For `BackgroundInitializer` I think the sensible thing to do is to use a `ConcurrentException` for `get()` and `close()` but not recast any exception during `initialize()` since that will be wrapped by Future anyway.
The above is for the outputs. For the inputs, I think the Supplier and Consumer should accept possible exception deceleration so you can plug in library methods without glue code, and because I think boilerplate like placing the real exception in a wrapper should be done in one central location if possible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Use LazyInitializer without subclassing. [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #1123:
URL: https://github.com/apache/commons-lang/pull/1123#issuecomment-1766209601
I agree with your point on the less elegant throwing of ConcurrentException. The issue is that we have three methods that can throw: get, initialize and close. Each could throw its own different Exception. I am trying to avoid declaring 3 generic parameter types and keep with just one. This might not be possible but OTOH the 3 parameter variant might be "too much".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Use LazyInitializer without subclassing. [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #1123:
URL: https://github.com/apache/commons-lang/pull/1123#discussion_r1361987175
##########
src/main/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializer.java:
##########
@@ -26,15 +32,128 @@
*/
public abstract class AbstractConcurrentInitializer<T, E extends Exception> implements ConcurrentInitializer<T> {
+ /**
+ * Builds a new instance for subclasses.
+ *
+ * @param <T> the type of the object managed by the initializer class.
+ * @param <I> the type of the initializer class.
+ * @param <B> the type of builder.
+ * @param <E> The exception type thrown by {@link #initialize()}.
+ */
+ public abstract static class AbstractBuilder<I extends AbstractConcurrentInitializer<T, E>, T, B extends AbstractBuilder<I, T, B, E>, E extends Exception>
+ extends AbstractSupplier<I, B, E> {
+
+ /**
+ * Closer consumer called by {@link #close()}.
+ */
+ private FailableConsumer<T, E> closer = FailableConsumer.nop();
+
+ /**
+ * Initializer supplier called by {@link #initialize()}.
+ */
+ private FailableSupplier<T, E> initializer = FailableSupplier.nul();
+
+ /**
+ * Gets the closer consumer called by {@link #close()}.
+ *
+ * @return the closer consumer called by {@link #close()}.
+ */
+ public FailableConsumer<T, E> getCloser() {
+ return closer;
+ }
+
+ /**
+ * Gets the initializer supplier called by {@link #initialize()}.
+ *
+ * @return the initializer supplier called by {@link #initialize()}.
+ */
+ public FailableSupplier<T, E> getInitializer() {
+ return initializer;
+ }
+
+ /**
+ * Sets the closer consumer called by {@link #close()}.
+ *
+ * @param closer the consumer called by {@link #close()}.
+ * @return this
+ */
+ public B setCloser(final FailableConsumer<T, E> closer) {
+ this.closer = closer != null ? closer : FailableConsumer.nop();
+ return asThis();
+ }
+
+ /**
+ * Sets the initializer supplier called by {@link #initialize()}.
+ *
+ * @param initializer the supplier called by {@link #initialize()}.
+ * @return this
+ */
+ public B setInitializer(final FailableSupplier<T, E> initializer) {
+ this.initializer = initializer != null ? initializer : FailableSupplier.nul();
+ return asThis();
+ }
+
+ }
+
+ /**
+ * Closer consumer called by {@link #close()}.
+ */
+ private final FailableConsumer<? super T, E> closer;
+
+ /**
+ * Initializer supplier called by {@link #initialize()}.
+ */
+ private final FailableSupplier<? extends T, E> initializer;
+
+ /**
+ * Constructs a new instance.
+ */
+ public AbstractConcurrentInitializer() {
+ this(FailableSupplier.nul(), FailableConsumer.nop());
+ }
+
+ /**
+ * Constructs a new instance.
+ *
+ * @param initializer the initializer supplier called by {@link #initialize()}.
+ * @param closer the closer consumer called by {@link #close()}.
+ */
+ AbstractConcurrentInitializer(final FailableSupplier<? extends T, E> initializer, final FailableConsumer<? super T, E> closer) {
+ this.closer = Objects.requireNonNull(closer, "closer");
+ this.initializer = Objects.requireNonNull(initializer, "initializer");
+ }
+
+ /**
+ * Calls the closer with the manager object.
+ *
+ * @throws E Thrown by the closer.
+ * @since 3.14.0
+ */
+ public void close() throws E {
+ if (isInitialized()) {
+ try {
+ closer.accept(get());
+ } catch (final ConcurrentException e) {
+ // Should not happen
+ throw new IllegalStateException(e);
+ }
+ }
+ }
+
/**
* Creates and initializes the object managed by this {@code
* ConcurrentInitializer}. This method is called by {@link #get()} when the object is accessed for the first time. An implementation can focus on the
* creation of the object. No synchronization is needed, as this is already handled by {@code get()}.
+ * <p>
+ * Subclasses and clients that do not an initializer are expected to implement this method.
Review Comment:
Fixed, ty.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Use LazyInitializer without subclassing. [commons-lang]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #1123:
URL: https://github.com/apache/commons-lang/pull/1123#issuecomment-1773084461
## [Codecov](https://app.codecov.io/gh/apache/commons-lang/pull/1123?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#1123](https://app.codecov.io/gh/apache/commons-lang/pull/1123?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (929a17d) into [master](https://app.codecov.io/gh/apache/commons-lang/commit/88879df3a9ba47d597f2620c1a72befcb5c69711?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (88879df) will **decrease** coverage by `0.06%`.
> Report is 11 commits behind head on master.
> The diff coverage is `85.89%`.
```diff
@@ Coverage Diff @@
## master #1123 +/- ##
============================================
- Coverage 92.21% 92.15% -0.06%
- Complexity 7571 7591 +20
============================================
Files 199 200 +1
Lines 15824 15902 +78
Branches 2918 2925 +7
============================================
+ Hits 14592 14655 +63
- Misses 661 675 +14
- Partials 571 572 +1
```
| [Files](https://app.codecov.io/gh/apache/commons-lang/pull/1123?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...apache/commons/lang3/builder/AbstractSupplier.java](https://app.codecov.io/gh/apache/commons-lang/pull/1123?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvbGFuZzMvYnVpbGRlci9BYnN0cmFjdFN1cHBsaWVyLmphdmE=) | `100.00% <100.00%> (ø)` | |
| [...he/commons/lang3/concurrent/AtomicInitializer.java](https://app.codecov.io/gh/apache/commons-lang/pull/1123?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvbGFuZzMvY29uY3VycmVudC9BdG9taWNJbml0aWFsaXplci5qYXZh) | `88.88% <100.00%> (+7.07%)` | :arrow_up: |
| [...ommons/lang3/concurrent/AtomicSafeInitializer.java](https://app.codecov.io/gh/apache/commons-lang/pull/1123?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvbGFuZzMvY29uY3VycmVudC9BdG9taWNTYWZlSW5pdGlhbGl6ZXIuamF2YQ==) | `94.11% <100.00%> (+4.11%)` | :arrow_up: |
| [...ache/commons/lang3/concurrent/LazyInitializer.java](https://app.codecov.io/gh/apache/commons-lang/pull/1123?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvbGFuZzMvY29uY3VycmVudC9MYXp5SW5pdGlhbGl6ZXIuamF2YQ==) | `94.73% <100.00%> (+3.07%)` | :arrow_up: |
| [...s/lang3/concurrent/MultiBackgroundInitializer.java](https://app.codecov.io/gh/apache/commons-lang/pull/1123?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvbGFuZzMvY29uY3VycmVudC9NdWx0aUJhY2tncm91bmRJbml0aWFsaXplci5qYXZh) | `100.00% <100.00%> (ø)` | |
| [...ang3/concurrent/CallableBackgroundInitializer.java](https://app.codecov.io/gh/apache/commons-lang/pull/1123?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvbGFuZzMvY29uY3VycmVudC9DYWxsYWJsZUJhY2tncm91bmRJbml0aWFsaXplci5qYXZh) | `91.66% <0.00%> (-8.34%)` | :arrow_down: |
| [...ang3/concurrent/AbstractConcurrentInitializer.java](https://app.codecov.io/gh/apache/commons-lang/pull/1123?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvbGFuZzMvY29uY3VycmVudC9BYnN0cmFjdENvbmN1cnJlbnRJbml0aWFsaXplci5qYXZh) | `92.85% <92.85%> (-7.15%)` | :arrow_down: |
| [...ommons/lang3/concurrent/BackgroundInitializer.java](https://app.codecov.io/gh/apache/commons-lang/pull/1123?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvbGFuZzMvY29uY3VycmVudC9CYWNrZ3JvdW5kSW5pdGlhbGl6ZXIuamF2YQ==) | `77.77% <11.11%> (-11.12%)` | :arrow_down: |
... and [2 files with indirect coverage changes](https://app.codecov.io/gh/apache/commons-lang/pull/1123/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Use LazyInitializer without subclassing. [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #1123:
URL: https://github.com/apache/commons-lang/pull/1123#issuecomment-1765267156
CC: @benjamin-confino
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Use LazyInitializer without subclassing. [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory merged PR #1123:
URL: https://github.com/apache/commons-lang/pull/1123
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Use LazyInitializer without subclassing. [commons-lang]
Posted by "benjamin-confino (via GitHub)" <gi...@apache.org>.
benjamin-confino commented on PR #1123:
URL: https://github.com/apache/commons-lang/pull/1123#issuecomment-1766350374
I've just finished testing locally, I'll PR your branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Use LazyInitializer without subclassing. [commons-lang]
Posted by "benjamin-confino (via GitHub)" <gi...@apache.org>.
benjamin-confino commented on code in PR #1123:
URL: https://github.com/apache/commons-lang/pull/1123#discussion_r1361893426
##########
src/main/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializer.java:
##########
@@ -26,15 +32,128 @@
*/
public abstract class AbstractConcurrentInitializer<T, E extends Exception> implements ConcurrentInitializer<T> {
+ /**
+ * Builds a new instance for subclasses.
+ *
+ * @param <T> the type of the object managed by the initializer class.
+ * @param <I> the type of the initializer class.
+ * @param <B> the type of builder.
+ * @param <E> The exception type thrown by {@link #initialize()}.
+ */
+ public abstract static class AbstractBuilder<I extends AbstractConcurrentInitializer<T, E>, T, B extends AbstractBuilder<I, T, B, E>, E extends Exception>
+ extends AbstractSupplier<I, B, E> {
+
+ /**
+ * Closer consumer called by {@link #close()}.
+ */
+ private FailableConsumer<T, E> closer = FailableConsumer.nop();
+
+ /**
+ * Initializer supplier called by {@link #initialize()}.
+ */
+ private FailableSupplier<T, E> initializer = FailableSupplier.nul();
+
+ /**
+ * Gets the closer consumer called by {@link #close()}.
+ *
+ * @return the closer consumer called by {@link #close()}.
+ */
+ public FailableConsumer<T, E> getCloser() {
+ return closer;
+ }
+
+ /**
+ * Gets the initializer supplier called by {@link #initialize()}.
+ *
+ * @return the initializer supplier called by {@link #initialize()}.
+ */
+ public FailableSupplier<T, E> getInitializer() {
+ return initializer;
+ }
+
+ /**
+ * Sets the closer consumer called by {@link #close()}.
+ *
+ * @param closer the consumer called by {@link #close()}.
+ * @return this
+ */
+ public B setCloser(final FailableConsumer<T, E> closer) {
+ this.closer = closer != null ? closer : FailableConsumer.nop();
+ return asThis();
+ }
+
+ /**
+ * Sets the initializer supplier called by {@link #initialize()}.
+ *
+ * @param initializer the supplier called by {@link #initialize()}.
+ * @return this
+ */
+ public B setInitializer(final FailableSupplier<T, E> initializer) {
+ this.initializer = initializer != null ? initializer : FailableSupplier.nul();
+ return asThis();
+ }
+
+ }
+
+ /**
+ * Closer consumer called by {@link #close()}.
+ */
+ private final FailableConsumer<? super T, E> closer;
+
+ /**
+ * Initializer supplier called by {@link #initialize()}.
+ */
+ private final FailableSupplier<? extends T, E> initializer;
+
+ /**
+ * Constructs a new instance.
+ */
+ public AbstractConcurrentInitializer() {
+ this(FailableSupplier.nul(), FailableConsumer.nop());
+ }
+
+ /**
+ * Constructs a new instance.
+ *
+ * @param initializer the initializer supplier called by {@link #initialize()}.
+ * @param closer the closer consumer called by {@link #close()}.
+ */
+ AbstractConcurrentInitializer(final FailableSupplier<? extends T, E> initializer, final FailableConsumer<? super T, E> closer) {
+ this.closer = Objects.requireNonNull(closer, "closer");
+ this.initializer = Objects.requireNonNull(initializer, "initializer");
+ }
+
+ /**
+ * Calls the closer with the manager object.
+ *
+ * @throws E Thrown by the closer.
+ * @since 3.14.0
+ */
+ public void close() throws E {
+ if (isInitialized()) {
+ try {
+ closer.accept(get());
+ } catch (final ConcurrentException e) {
+ // Should not happen
+ throw new IllegalStateException(e);
+ }
+ }
+ }
+
/**
* Creates and initializes the object managed by this {@code
* ConcurrentInitializer}. This method is called by {@link #get()} when the object is accessed for the first time. An implementation can focus on the
* creation of the object. No synchronization is needed, as this is already handled by {@code get()}.
+ * <p>
+ * Subclasses and clients that do not an initializer are expected to implement this method.
Review Comment:
There is a missing word "provide" after "do not".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Use LazyInitializer without subclassing. [commons-lang]
Posted by "benjamin-confino (via GitHub)" <gi...@apache.org>.
benjamin-confino commented on PR #1123:
URL: https://github.com/apache/commons-lang/pull/1123#issuecomment-1766175557
I do have one thought. Currently when you create a supplier for LazyInitializer the type signature means that it must declare no exceptions or declare `throws ConcurrentException`. I think this is a serious limitation. It means you cannot do something like `LazyInitializer.builder().setInitializer(SomeLibrary::methodWithCheckedException)` without writing boilerplate code to catch that exception and wrap it.
This can be solved by making the supplier and consumer take an exception of `? extends Exception` and then casting it as necessary inside `initialize()`. I think the cleanest solution would be to have an abstract method like `getExceptionType` that initialize could use to tell what it needs to cast an exception into. I'll write it locally and test to see if it works.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org