Nuno,
I think you are right. The problem with deprecating dispose is that the
default method uses it and we will never be rid of it. What we really
want is for implementers to use dispose() and for only
try-with-resources to use close(). Java does not permit final on default
methods
<https://stackoverflow.com/questions/23453287/why-is-final-not-allowed-in-java-8-interface-methods>
but we can add a stern warning to treat close() as final. We could even
deprecate close(), which is an ugly misuse, but might be the best way of
warning implementers.
I would also remove the exception specification because it can never be
thrown, and for consistency with dispose().
To make intent clear and make retrofitting even easier, I propose that
we add a Disposable interface that we can conveniently add to any
interface or class with an existing dispose() method. How about:
public interface Disposable extends AutoCloseable {
/**
* Stern warning to treat close as final */
* @see java.lang.AutoCloseable#close()
*/
@Deprecated
@Override
default void close() {
dispose();
}
void dispose();
}
I would put this in org.geotools.util in gt-metadata so it can be used
by the classes in gt-referencing that have dispose().
The one class that cannot be fixed by this is the one that motivated me
to start: ImageReader, which is in javax.imageio, and thus outside our
grasp. This will require some ugly workarounds including delegates and
static factory methods, but this should not deter us from the above fix.
I am calling ImageReader out of scope for this change proposal.
Kind regards,
Ben.
On 05/06/18 20:02, Nuno Oliveira wrote:
I would add the AutoClose interface to all interfaces that have the
dispose method or a similar one. Then I would provide a default
implementation for the close method that invokes the dispose method:
default void close() throws Exception {
dispose();
}
This would make the interfaces fully backward compatible and would allow
us to use the resource try catch pattern. I don't see any possible
resource leakage with this approach, the new code that will start using
the auto close approach will delegate on the existing dispose method and
old code will still use the dispose method.
The only drawback I see is that the dispose method would still around,
the only thing we could do is mark it as deprecated ... but I can leave
with that.
On 06/05/2018 02:16 AM, Ben Caradoc-Davies wrote:
Erik,
we require Java 8 for all supported branches. Interface default
methods are on the table.
Kind regards,
Ben.
On 05/06/18 12:54, Erik Merkle wrote:
A small caveat to my suggestion about default methods. Apparently
default
methods on interfaces is a Java 8 thing. So it is not a viable option if
running with an older version.
Erik Merkle
Software Engineer | Boundless
<http://d32wfbeasdaw38.cloudfront.net/img/Boundless_Logo.png>
On Mon, Jun 4, 2018 at 7:49 PM, Erik Merkle <emer...@boundlessgeo.com>
wrote:
I don't believe I have a vote here, but I wanted to add that you could
provide a default implementation on any GeoServer Interfaces to
which you
want to add AutoCloseable. That would get around the compile issue,
and I
believe the backward compatibility compile issue is exactly why Java
added
the default keyword for interface methods. The default
implementation could
simply be a no-op for things like close().
While it would alleviate compile issues, it might not have the most
reliable runtime effects.
For what it's worth,
Erik Merkle
Software Engineer | Boundless
<http://d32wfbeasdaw38.cloudfront.net/img/Boundless_Logo.png>
On Mon, Jun 4, 2018 at 6:24 PM, Ben Caradoc-Davies <b...@transient.nz>
wrote:
Many interfaces in GeoTools and GeoServer use the Dispose pattern,
often
with a dispose() method, but do not implement AutoCloseable,
preventing
their use in a try-with-resources statement. Examples range from
ImageReader to DataStore/DataAccess. Some interfaces like
FeatureReader
already implement Closeable and thus AutoCloseable, but many do not.
Java 7 try-with-resources improves code quality because it simplifies
code by automating common boilerplate:
https://docs.oracle.com/javase/tutorial/essential/exceptions
/tryResourceClose.html
https://docs.oracle.com/javase/8/docs/api/java/lang/AutoCloseable.html
Adding AutoCloseable to an interface is an API-breaking change because
third-party subclasses that do not implement a close() method will no
longer compile. Any change would be applied only to master and
would target
GeoTools 20.0 and GeoServer 2.14.0.
- Should we add AutoCloseable to interfaces, and if so which ones? We
could make a list.
- Do we make the change one interface at a time or try to do them
all at
once?
- Should we rename dispose() to close() in implementers and add a
deprecated dispose() that wraps close(), or just add a close() that
wraps
dispose()?
- As we are breaking the API anyway, should we get rid of dispose()
entirely by renaming it to close() without adding a deprecated
wrapper?
- I thought of updating only interfaces and overrides. A more
ambitious
scope would find every deprecated dispose() and refactor to use
try-with-resources. The alternative is to refactor incrementally
over time.
How do we wish to pay off our technical debt?
- Who is interested in participating in this work?
Kind regards,
--
Ben Caradoc-Davies <b...@transient.nz>
Director
Transient Software Limited <https://transient.nz/>
New Zealand
------------------------------------------------------------
------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Geoserver-devel mailing list
geoserver-de...@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel
--
Ben Caradoc-Davies <b...@transient.nz>
Director
Transient Software Limited <https://transient.nz/>
New Zealand
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
GeoTools-Devel mailing list
GeoTools-Devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geotools-devel
