[Geoserver-devel] API changes to add AutoCloseable for try-with-resources

GeoServer GeoServer Developer
1

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 <ben@anonymised.com>
Director
Transient Software Limited <https://transient.nz/&gt;
New Zealand

2

Ben,

Something to potentially consider regarding interfaces; While changing the signatures of existing interfaces will break the API, you could do one of two things:
1. copy the current interface wholesale and change the name to include a digit i.e. MyInterface => MyInterface1
Where the new interface name has the updated signature/implements. Implementations of the interface can then change to implement the newly name one, while existing implementations continue to work against the original interface. The original interface can be marked as "@Deprecated use interface XXXX" and eventually removed altogether.

2. create a new interface that extends the original interface adding the additional changes. This also uses the same process for implementation classes, but has the downside of NOT being able to deprecate/remove the original names if desired.

Anyway, something to consider.

Chris Snider
Senior Software Engineer

-----Original Message-----
From: Ben Caradoc-Davies [mailto:ben@anonymised.com]
Sent: Monday, June 04, 2018 5:25 PM
To: GeoTools Devel <geotools-devel@lists.sourceforge.net>; Geoserver-devel <geoserver-devel@lists.sourceforge.net>
Subject: [Geoserver-devel] API changes to add AutoCloseable for try-with-resources

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 <ben@anonymised.com>
Director
Transient Software Limited <https://transient.nz/&gt;
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-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

3

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,

On Mon, Jun 4, 2018 at 6:24 PM, Ben Caradoc-Davies <ben@anonymised.com> 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 <ben@anonymised.com>
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-devel@anonymised.comrge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

Erik Merkle
Software Engineer | Boundless

4

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.

···

On Mon, Jun 4, 2018 at 7:49 PM, Erik Merkle <emerkle@anonymised.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.

Erik Merkle
Software Engineer | Boundless

​For what it’s worth,

Erik Merkle
Software Engineer | Boundless

On Mon, Jun 4, 2018 at 6:24 PM, Ben Caradoc-Davies <ben@anonymised.com> 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 <ben@anonymised.com>
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-devel@anonymised.comrge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

5

Eric,

that is an interesting idea but I suspect it would introduce silent resource leaks. Unless we have a default implementation that calls the deprecated dispose() method? In any case, just like constructors and virtual destructors in C++, there in an intimate relationship between a close() method and the implementing class. Avoiding a default implementation avoids the risk that an implementer might forget to override it.

Kind regards,
Ben.

On 05/06/18 12:49, Erik Merkle 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&gt;

On Mon, Jun 4, 2018 at 6:24 PM, Ben Caradoc-Davies <ben@anonymised.com> 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 <ben@anonymised.com>
Director
Transient Software Limited <https://transient.nz/&gt;
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-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

--
Ben Caradoc-Davies <ben@anonymised.com>
Director
Transient Software Limited <https://transient.nz/&gt;
New Zealand

6

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&gt;

On Mon, Jun 4, 2018 at 7:49 PM, Erik Merkle <emerkle@anonymised.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&gt;

On Mon, Jun 4, 2018 at 6:24 PM, Ben Caradoc-Davies <ben@anonymised.com>
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 <ben@anonymised.com>
Director
Transient Software Limited <https://transient.nz/&gt;
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-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

--
Ben Caradoc-Davies <ben@anonymised.com>
Director
Transient Software Limited <https://transient.nz/&gt;
New Zealand

7

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&gt;

On Mon, Jun 4, 2018 at 7:49 PM, Erik Merkle <emerkle@anonymised.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&gt;

On Mon, Jun 4, 2018 at 6:24 PM, Ben Caradoc-Davies <ben@anonymised.com>
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 <ben@anonymised.com>
Director
Transient Software Limited <https://transient.nz/&gt;
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-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

--
Regards,
Nuno Oliveira

GeoServer Professional Services from the experts!
Visit http://goo.gl/it488V for more information.

Nuno Miguel Carvalho Oliveira
@nmcoliveira
Software Engineer

GeoSolutions S.A.S.
Via di Montramito 3/A
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

-------------------------------------------------------

Con riferimento alla normativa sul trattamento dei dati
personali (Reg. UE 2016/679 - Regolamento generale sulla
protezione dei dati “GDPR”), si precisa che ogni
circostanza inerente alla presente email (il suo contenuto,
gli eventuali allegati, etc.) è un dato la cui conoscenza
è riservata al/i solo/i destinatario/i indicati dallo
scrivente. Se il messaggio Le è giunto per errore, è
tenuta/o a cancellarlo, ogni altra operazione è illecita.
Le sarei comunque grato se potesse darmene notizia.

This email is intended only for the person or entity to
which it is addressed and may contain information that
is privileged, confidential or otherwise protected from
disclosure. We remind that - as provided by European
Regulation 2016/679 “GDPR” - copying, dissemination or
use of this e-mail or the information herein by anyone
other than the intended recipient is prohibited. If you
have received this email by mistake, please notify
us immediately by telephone or e-mail.

8

On Tue, 5 Jun 2018 at 00:25, Ben Caradoc-Davies <ben@anonymised.com> 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.

+1 for this anything that improves clean up and saves me writing code is good!

  • Should we add AutoCloseable to interfaces, and if so which ones? We
    could make a list.

This is an obvious first step, is there an easy way to do it? grep for dispose?

  • Do we make the change one interface at a time or try to do them all at
    once?

I would go for one at a time, working from a list. My feeling is that doing one is something I could work on while waiting for something else to complete, whereas doing all of them is going to be a weekend or more and harder to share the work out.

  • Should we rename dispose() to close() in implementers and add a
    deprecated dispose() that wraps close(), or just add a close() that
    wraps dispose()?

I’d favour deprecating a dispose() that wraps close() - makes it clearer what we intend.

  • As we are breaking the API anyway, should we get rid of dispose()
    entirely by renaming it to close() without adding a deprecated wrapper?

That is harder for downstream users to handle IMHO.

  • 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?

I would prefer to fight the debt as we go and find all the deprecated disposes in our code as we go.

  • Who is interested in participating in this work?

I’d be up to do some of it.

Ian

9

On Tue, Jun 5, 2018 at 10:05 AM, Ian Turton <ijturton@anonymised.com> wrote:

- Who is interested in participating in this work?

I'd be up to do some of it.

Same here. As spare time contribution, if we are a group, we do the
deprecation, and attack one interface at a time.
As a code sprint if we want to do all toghether (though, in a week long
code sprint, we might want to look into more
"urgent yet too big" stuff, like running the whole project stack on Java
10-11).

Cheers
Andrea

== GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information. == Ing. Andrea Aime @geowolf
Technical Lead GeoSolutions S.A.S. Via di Montramito 3/A 55054 Massarosa
(LU) phone: +39 0584 962313 fax: +39 0584 1660272 mob: +39 339 8844549
http://www.geo-solutions.it http://twitter.com/geosolutions_it
------------------------------------------------------- *Con riferimento
alla normativa sul trattamento dei dati personali (Reg. UE 2016/679 -
Regolamento generale sulla protezione dei dati “GDPR”), si precisa che ogni
circostanza inerente alla presente email (il suo contenuto, gli eventuali
allegati, etc.) è un dato la cui conoscenza è riservata al/i solo/i
destinatario/i indicati dallo scrivente. Se il messaggio Le è giunto per
errore, è tenuta/o a cancellarlo, ogni altra operazione è illecita. Le
sarei comunque grato se potesse darmene notizia. This email is intended
only for the person or entity to which it is addressed and may contain
information that is privileged, confidential or otherwise protected from
disclosure. We remind that - as provided by European Regulation 2016/679
“GDPR” - copying, dissemination or use of this e-mail or the information
herein by anyone other than the intended recipient is prohibited. If you
have received this email by mistake, please notify us immediately by
telephone or e-mail.*

10

No voting rights here, but just wanted to express my support for this!

On 05-06-18 10: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&gt;

On Mon, Jun 4, 2018 at 7:49 PM, Erik Merkle <emerkle@anonymised.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&gt;

On Mon, Jun 4, 2018 at 6:24 PM, Ben Caradoc-Davies <ben@anonymised.com>
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 <ben@anonymised.com>
Director
Transient Software Limited <https://transient.nz/&gt;
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-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

11

A big +1 one from my side, the auto closable feature increases code quality.

Cheers

···

On Tue, Jun 5, 2018 at 10:47 AM, Niels Charlier <niels@anonymised.com> wrote:

No voting rights here, but just wanted to express my support for this!

On 05-06-18 10: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 <emerkle@anonymised.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 <ben@anonymised.com>
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 <ben@anonymised.com>
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-devel@anonymised.comrge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

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-devel@anonymised.comrge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

–

DI Christian Mueller MSc (GIS), MSc (IT-Security)
OSS Open Source Solutions GmbH

12

On 05/06/18 20:05, Ian Turton wrote:

On Tue, 5 Jun 2018 at 00:25, Ben Caradoc-Davies <ben@anonymised.com> wrote:

- Should we add AutoCloseable to interfaces, and if so which ones? We
could make a list.

This is an obvious first step, is there an easy way to do it? grep for
dispose?

I found many interfaces and classes with:
find . -name "*.java" -exec grep -H 'void dispose()' {} \;

With context:
find . -name "*.java" -exec grep -H -B2 'void dispose()' {} \;

Names only:
find . -name "*.java" -exec grep -l 'void dispose()' {} \; | sort

Nuno also made a list.

- Should we rename dispose() to close() in implementers and add a
deprecated dispose() that wraps close(), or just add a close() that
wraps dispose()?

I'd favour deprecating a dispose() that wraps close() - makes it clearer
what we intend.

This was my first thought, but now I prefer Nuno and Erik's suggestion of a default method. I am contemplating adding a Disposable interface.

Kind regards,

--
Ben Caradoc-Davies <ben@anonymised.com>
Director
Transient Software Limited <https://transient.nz/&gt;
New Zealand

13

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&gt; 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&gt;

On Mon, Jun 4, 2018 at 7:49 PM, Erik Merkle <emerkle@anonymised.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&gt;

On Mon, Jun 4, 2018 at 6:24 PM, Ben Caradoc-Davies <ben@anonymised.com>
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 <ben@anonymised.com>
Director
Transient Software Limited <https://transient.nz/&gt;
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-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

--
Ben Caradoc-Davies <ben@anonymised.com>
Director
Transient Software Limited <https://transient.nz/&gt;
New Zealand

14

On 06/06/18 13:48, Ben Caradoc-Davies wrote:

I would also remove the exception specification because it can never be thrown, and for consistency with dispose().

Although I should note that there are several classes with a dispose() that throws. While this is ugly, it can be a necessary guard required for data integrity. We might need two interfaces ThrowingDisposable and Disposable:

public interface ThrowingDisposable extends AutoCloseable {
     /**
      * @see java.lang.AutoCloseable#close()
      */
     @Deprecated
     @Override
     default void close() throws Exception {
         dispose();
     }
     void dispose() throws Exception;
}

public interface Disposable extends ThrowingDisposable {
     /**
      * @see java.lang.AutoCloseable#close()
      */
     @Deprecated
     @Override
     default void close() {
         dispose();
     }
     @Override
     void dispose();
}

Kind regards,

--
Ben Caradoc-Davies <ben@anonymised.com>
Director
Transient Software Limited <https://transient.nz/&gt;
New Zealand

15

Or with generics:

public interface ThrowingDisposable<T extends Exception> extends AutoCloseable {
     /**
      * @see java.lang.AutoCloseable#close()
      */
     @Deprecated
     @Override
     default void close() throws T {
         dispose();
     }
     void dispose() throws T;
}
public interface Disposable extends ThrowingDisposable<Exception> {
     /**
      * @see java.lang.AutoCloseable#close()
      */
     @Deprecated
     @Override
     default void close() {
         dispose();
     }
     @Override
     void dispose();
}

Or just keep it simple and have dispose throw Exception like java.lang.AutoCloseable#close() and let implementers narrow the return type to no exception if they like. Impact will be minimal as client code will likely use an implementer not a bare Disposable:

public interface Disposable extends AutoCloseable{
     /**
      * @throws Exception
      * @see java.lang.AutoCloseable#close()
      */
     @Deprecated
     @Override
     default void close() throws Exception{
         dispose();
     }
     void dispose() throws Exception;
}

Preference?

Kind regards,

--
Ben Caradoc-Davies <ben@anonymised.com>
Director
Transient Software Limited <https://transient.nz/&gt;
New Zealand

16

Hi Ben,

+1 for the NON generics option, my felling is that usually generics bring more harm than good in the long run :frowning:

On 06/06/2018 03:44 AM, Ben Caradoc-Davies wrote:

Or with generics:

public interface ThrowingDisposable<T extends Exception> extends AutoCloseable {
/**
* @see java.lang.AutoCloseable#close()
*/
@Deprecated
@Override
default void close() throws T {
dispose();
}
void dispose() throws T;
}
public interface Disposable extends ThrowingDisposable<Exception> {
/**
* @see java.lang.AutoCloseable#close()
*/
@Deprecated
@Override
default void close() {
dispose();
}
@Override
void dispose();
}

Or just keep it simple and have dispose throw Exception like java.lang.AutoCloseable#close() and let implementers narrow the return type to no exception if they like. Impact will be minimal as client code will likely use an implementer not a bare Disposable:

public interface Disposable extends AutoCloseable{
/**
* @throws Exception
* @see java.lang.AutoCloseable#close()
*/
@Deprecated
@Override
default void close() throws Exception{
dispose();
}
void dispose() throws Exception;
}

Preference?

Kind regards,

--
Regards,
Nuno Oliveira

GeoServer Professional Services from the experts!
Visit http://goo.gl/it488V for more information.

Nuno Miguel Carvalho Oliveira
@nmcoliveira
Software Engineer

GeoSolutions S.A.S.
Via di Montramito 3/A
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

-------------------------------------------------------

Con riferimento alla normativa sul trattamento dei dati
personali (Reg. UE 2016/679 - Regolamento generale sulla
protezione dei dati “GDPR”), si precisa che ogni
circostanza inerente alla presente email (il suo contenuto,
gli eventuali allegati, etc.) è un dato la cui conoscenza
è riservata al/i solo/i destinatario/i indicati dallo
scrivente. Se il messaggio Le è giunto per errore, è
tenuta/o a cancellarlo, ogni altra operazione è illecita.
Le sarei comunque grato se potesse darmene notizia.

This email is intended only for the person or entity to
which it is addressed and may contain information that
is privileged, confidential or otherwise protected from
disclosure. We remind that - as provided by European
Regulation 2016/679 “GDPR” - copying, dissemination or
use of this e-mail or the information herein by anyone
other than the intended recipient is prohibited. If you
have received this email by mistake, please notify
us immediately by telephone or e-mail.

17

Agreed, and Java generics are erased at compile time and offer no run time benefits.

Kind regards,
Ben.

On 07/06/18 08:06, Nuno Oliveira wrote:

Hi Ben,

+1 for the NON generics option, my felling is that usually generics bring more harm than good in the long run :frowning:

On 06/06/2018 03:44 AM, Ben Caradoc-Davies wrote:

Or with generics:

public interface ThrowingDisposable<T extends Exception> extends AutoCloseable {
/**
* @see java.lang.AutoCloseable#close()
*/
@Deprecated
@Override
default void close() throws T {
dispose();
}
void dispose() throws T;
}
public interface Disposable extends ThrowingDisposable<Exception> {
/**
* @see java.lang.AutoCloseable#close()
*/
@Deprecated
@Override
default void close() {
dispose();
}
@Override
void dispose();
}

Or just keep it simple and have dispose throw Exception like java.lang.AutoCloseable#close() and let implementers narrow the return type to no exception if they like. Impact will be minimal as client code will likely use an implementer not a bare Disposable:

public interface Disposable extends AutoCloseable{
/**
* @throws Exception
* @see java.lang.AutoCloseable#close()
*/
@Deprecated
@Override
default void close() throws Exception{
dispose();
}
void dispose() throws Exception;
}

Preference?

Kind regards,

--
Ben Caradoc-Davies <ben@anonymised.com>
Director
Transient Software Limited <https://transient.nz/&gt;
New Zealand