FileDescriptor
overrides finalize
:
https://github.com/spring-projects/spring-boot/blob/9c9fbf400e11c40cd10b883cf986a9d4a65009e4/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/main/java/org/springframework/boot/buildpack/platform/socket/FileDescriptor.java#L47-L50
I'm not sure that it's needed as DomainSocket
– the only place where FileDescriptor
is used – will close it when the socket is closed:
https://github.com/spring-projects/spring-boot/blob/9c9fbf400e11c40cd10b883cf986a9d4a65009e4/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/main/java/org/springframework/boot/buildpack/platform/socket/DomainSocket.java#L113-L122
If the DomainSocket
isn't being closed for some reason then we'd be leaking its input and output streams as the super.close()
call is what causes them to be closed. I suspect we can just remove the override of finalize()
completely. This'll help with 3.0 and the Java 17 baseline where finalize()
has been deprecated. Flagging for team attention to see if I've missed something and it's needed.
Comment From: dreis2211
I wonder if this ticket could be extended to review the use of finalize()
in general. There is another one in RestartClassLoader
that seems not to be strictly needed:
https://github.com/spring-projects/spring-boot/blob/1a505964ee5e1c68cb85e6cc2d99a7dbf9835677/spring-boot-project/spring-boot-devtools/src/main/java/org/springframework/boot/devtools/restart/classloader/RestartClassLoader.java#L191-L196
Comment From: philwebb
I think I was being super defensive adding the finalizer
to FileDescriptor
. I agree that we can drop it. I guess we don’t need the one in RestartClassLoader
either. I probably added that one to check we didn’t have a restart memory leak.
Not sure if we should remove them in 2.4.x or not. Do they cause issues there?
Comment From: wilkinsona
All of the reasons given for deprecating finalize()
in Java 9 apply to Java 8 and earlier as well so I don't think we should wait unless we really feel the risk in a maintenance release is unacceptably high.
AIUI, there's also a non-zero cost to overriding finalize()
. Whenever an object that does so is created, a java.lang.ref.Finalizer
is also created and, once the object's being GCed, it gets added to a reference queue from where it'll eventually be taken and finalize()
called. While this is more of a problem when creating and GCing many such objects, I still think it's something that's worth avoiding.
I am in favour of removing the use of finalize()
in 2.4.x as I think the risk is lower than the benefit.
Comment From: philwebb
Sounds good. 2.4.x it is then.