Avoid flush in finalizers for Socket and IO::FileDescriptor

spec/std/io/file_descriptor_spec.cr

@@ -48,17 +48,33 @@ describe IO::FileDescriptor do
     end
   end
 
-  it "closes on finalize" do
-    pipes = [] of IO::FileDescriptor
-    assert_finalizes("fd") do
-      a, b = IO.pipe
-      pipes << b
-      a
+  describe "#finalize" do
+    it "closes" do
+      pipes = [] of IO::FileDescriptor
+      assert_finalizes("fd") do
+        a, b = IO.pipe
+        pipes << b
+        a
+      end
+
+      expect_raises(IO::Error) do
+        pipes.each do |p|
+          p.puts "123"
+        end
+      end
     end
 
-    expect_raises(IO::Error) do
-      pipes.each do |p|
-        p.puts "123"
+    it "does not flush" do
+      with_tempfile "fd-finalize-flush" do |path|
+        file = File.new(path, "w")
+        file << "foo"
+        file.flush
+        file << "bar"
+        file.finalize
+
+        File.read(path).should eq "foo"
+      ensure
+        file.try(&.close) rescue nil
       end
     end
   end

spec/std/socket/socket_spec.cr

@@ -169,4 +169,32 @@ describe Socket, tags: "network" do
       socket.close_on_exec?.should be_true
     end
   {% end %}
+
+  describe "#finalize" do
+    it "does not flush" do
+      port = unused_local_port
+      server = Socket.tcp(Socket::Family::INET)
+      server.bind("127.0.0.1", port)
+      server.listen
+
+      spawn do
+        client = server.not_nil!.accept
+        client.sync = false
+        client << "foo"
+        client.flush
+        client << "bar"
+        client.finalize
+      ensure
+        client.try(&.close) rescue nil
+      end
+
+      socket = Socket.tcp(Socket::Family::INET)
+      socket.connect(Socket::IPAddress.new("127.0.0.1", port))
+
+      socket.gets.should eq "foo"
+    ensure
+      socket.try &.close
+      server.try &.close
+    end
+  end
 end

src/crystal/system/file.cr

@@ -87,13 +87,6 @@ module Crystal::System::File
   private def self.error_is_file_exists?(errno)
     errno.in?(Errno::EEXIST, WinError::ERROR_FILE_EXISTS)
   end
-
-  # Closes the internal file descriptor without notifying libevent.
-  # This is directly used after the fork of a process to close the
-  # parent's Crystal::System::Signal.@@pipe reference before re initializing
-  # the event loop. In the case of a fork that will exec there is even
-  # no need to initialize the event loop at all.
-  # def file_descriptor_close
 end
 
 {% if flag?(:wasi) %}

src/crystal/system/file_descriptor.cr

@@ -14,6 +14,14 @@ module Crystal::System::FileDescriptor
   # cooked mode otherwise.
   # private def system_raw(enable : Bool, & : ->)
 
+  # Closes the internal file descriptor without notifying the event loop.
+  # This is directly used after the fork of a process to close the
+  # parent's Crystal::System::Signal.@@pipe reference before re initializing
+  # the event loop. In the case of a fork that will exec there is even
+  # no need to initialize the event loop at all.
+  # Also used in `IO::FileDescriptor#finalize`.
+  # def file_descriptor_close
+
   private def system_read(slice : Bytes) : Int32
     event_loop.read(self, slice)
   end

src/crystal/system/socket.cr

@@ -91,6 +91,14 @@ module Crystal::System::Socket
 
   # private def system_close
 
+  # Closes the internal handle without notifying the event loop.
+  # This is directly used after the fork of a process to close the
+  # parent's Crystal::System::Signal.@@pipe reference before re initializing
+  # the event loop. In the case of a fork that will exec there is even
+  # no need to initialize the event loop at all.
+  # Also used in `Socket#finalize`
+  # def socket_close
+
   private def event_loop : Crystal::EventLoop::Socket
     Crystal::EventLoop.current
   end

src/crystal/system/unix/file_descriptor.cr

@@ -120,7 +120,7 @@ module Crystal::System::FileDescriptor
     file_descriptor_close
   end
 
-  def file_descriptor_close : Nil
+  def file_descriptor_close(&) : Nil
     # Clear the @volatile_fd before actually closing it in order to
     # reduce the chance of reading an outdated fd value
     _fd = @volatile_fd.swap(-1)
@@ -130,11 +130,17 @@ module Crystal::System::FileDescriptor
       when Errno::EINTR, Errno::EINPROGRESS
         # ignore
       else
-        raise IO::Error.from_errno("Error closing file", target: self)
+        yield
       end
     end
   end
 
+  def file_descriptor_close
+    file_descriptor_close do
+      raise IO::Error.from_errno("Error closing file", target: self)
+    end
+  end
+
   private def system_flock_shared(blocking)
     flock LibC::FlockOp::SH, blocking
   end

src/crystal/system/unix/socket.cr

@@ -208,6 +208,10 @@ module Crystal::System::Socket
     # always lead to undefined results. This is not specific to libevent.
     event_loop.close(self)
 
+    socket_close
+  end
+
+  private def socket_close(&)
     # Clear the @volatile_fd before actually closing it in order to
     # reduce the chance of reading an outdated fd value
     fd = @volatile_fd.swap(-1)
@@ -219,11 +223,17 @@ module Crystal::System::Socket
       when Errno::EINTR, Errno::EINPROGRESS
         # ignore
       else
-        raise ::Socket::Error.from_errno("Error closing socket")
+        yield
       end
     end
   end
 
+  private def socket_close
+    socket_close do
+      raise ::Socket::Error.from_errno("Error closing socket")
+    end
+  end
+
   private def system_local_address
     sockaddr6 = uninitialized LibC::SockaddrIn6
     sockaddr = pointerof(sockaddr6).as(LibC::Sockaddr*)

src/crystal/system/win32/file_descriptor.cr

@@ -178,6 +178,12 @@ module Crystal::System::FileDescriptor
 
   def file_descriptor_close
     if LibC.CloseHandle(windows_handle) == 0
+      yield
+    end
+  end
+
+  def file_descriptor_close
+    file_descriptor_close do
       raise IO::Error.from_winerror("Error closing file", target: self)
     end
   end

src/crystal/system/win32/socket.cr

@@ -366,6 +366,10 @@ module Crystal::System::Socket
   end
 
   def system_close
+    socket_close
+  end
+
+  private def socket_close
     handle = @volatile_fd.swap(LibC::INVALID_SOCKET)
 
     ret = LibC.closesocket(handle)
@@ -375,11 +379,17 @@ module Crystal::System::Socket
       when WinError::WSAEINTR, WinError::WSAEINPROGRESS
         # ignore
       else
-        raise ::Socket::Error.from_os_error("Error closing socket", err)
+        yield err
       end
     end
   end
 
+  def socket_close
+    socket_close do |err|
+      raise ::Socket::Error.from_os_error("Error closing socket", err)
+    end
+  end
+
   private def system_local_address
     sockaddr6 = uninitialized LibC::SockaddrIn6
     sockaddr = pointerof(sockaddr6).as(LibC::Sockaddr*)

src/io/file_descriptor.cr

@@ -233,10 +233,21 @@ class IO::FileDescriptor < IO
     system_flock_unlock
   end
 
+  # Finalizes the file descriptor resource.
+  #
+  # This involves releasing the handle to the operating system, i.e. closing it.
+  # It does *not* implicitly call `#flush`, so data waiting in the buffer may be
+  # lost.
+  # It's recommended to always close the file descriptor explicitly via `#close`
+  # (or implicitly using the `.open` constructor).
+  #
+  # Resource release can be disabled with `close_on_finalize = false`.
+  #
+  # This method is a no-op if the file descriptor has already been closed.
   def finalize
     return if closed? || !close_on_finalize?
 
-    close rescue nil
+    file_descriptor_close { } # ignore error
   end
 
   def closed? : Bool

src/socket.cr

@@ -419,10 +419,18 @@ class Socket < IO
     self.class.fcntl fd, cmd, arg
   end
 
+  # Finalizes the socket resource.
+  #
+  # This involves releasing the handle to the operating system, i.e. closing it.
+  # It does *not* implicitly call `#flush`, so data waiting in the buffer may be
+  # lost. By default write buffering is disabled, though (`sync? == true`).
+  # It's recommended to always close the socket explicitly via `#close`.
+  #
+  # This method is a no-op if the file descriptor has already been closed.
   def finalize
     return if closed?
 
-    close rescue nil
+    socket_close { } # ignore error
   end
 
   def closed? : Bool