From 0bdcd4b1fab6dd0de646c96e655a214539f9eb87 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Johannes=20M=C3=BCller?= <straightshoota@gmail.com>
Date: Thu, 8 Aug 2024 14:23:31 +0200
Subject: [PATCH 1/7] Add spec for `IO::FileDescriptor#finalize` flushes

---
 spec/std/io/file_descriptor_spec.cr | 34 +++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/spec/std/io/file_descriptor_spec.cr b/spec/std/io/file_descriptor_spec.cr
index e497ac1061a3..91e1ffbaf28a 100644
--- a/spec/std/io/file_descriptor_spec.cr
+++ b/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 "flushes" 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 "foobar"
+      ensure
+        file.try(&.close) rescue nil
       end
     end
   end

From 69345f226f51af9bea6df17a2b7c2ecadf9e651b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Johannes=20M=C3=BCller?= <straightshoota@gmail.com>
Date: Thu, 8 Aug 2024 14:23:52 +0200
Subject: [PATCH 2/7] Add spec for `Socket#finalize` flushes

---
 spec/std/socket/socket_spec.cr | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/spec/std/socket/socket_spec.cr b/spec/std/socket/socket_spec.cr
index d4e7051d12bd..ef53bad71da3 100644
--- a/spec/std/socket/socket_spec.cr
+++ b/spec/std/socket/socket_spec.cr
@@ -169,4 +169,31 @@ describe Socket, tags: "network" do
       socket.close_on_exec?.should be_true
     end
   {% end %}
+
+  describe "#finalize" do
+    it "flushes" 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 "foobar"
+    ensure
+      socket.try &.close
+      server.try &.close
+    end
+  end
 end

From 668de3b793a7036ddc2029a175aac168b425ad75 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Johannes=20M=C3=BCller?= <straightshoota@gmail.com>
Date: Thu, 8 Aug 2024 14:55:46 +0200
Subject: [PATCH 3/7] Avoid flush in Socket and IO::FileDescriptor finalizer

---
 spec/std/io/file_descriptor_spec.cr         |  4 ++--
 spec/std/socket/socket_spec.cr              |  5 +++--
 src/crystal/system/unix/file_descriptor.cr  | 10 ++++++++--
 src/crystal/system/unix/socket.cr           | 12 +++++++++++-
 src/crystal/system/win32/file_descriptor.cr |  6 ++++++
 src/crystal/system/win32/socket.cr          |  8 +++++++-
 src/io/file_descriptor.cr                   |  2 +-
 src/socket.cr                               |  2 +-
 8 files changed, 39 insertions(+), 10 deletions(-)

diff --git a/spec/std/io/file_descriptor_spec.cr b/spec/std/io/file_descriptor_spec.cr
index 91e1ffbaf28a..2e10ea99c030 100644
--- a/spec/std/io/file_descriptor_spec.cr
+++ b/spec/std/io/file_descriptor_spec.cr
@@ -64,7 +64,7 @@ describe IO::FileDescriptor do
       end
     end
 
-    it "flushes" do
+    it "does not flush" do
       with_tempfile "fd-finalize-flush" do |path|
         file = File.new(path, "w")
         file << "foo"
@@ -72,7 +72,7 @@ describe IO::FileDescriptor do
         file << "bar"
         file.finalize
 
-        File.read(path).should eq "foobar"
+        File.read(path).should eq "foo"
       ensure
         file.try(&.close) rescue nil
       end
diff --git a/spec/std/socket/socket_spec.cr b/spec/std/socket/socket_spec.cr
index ef53bad71da3..2127e196b746 100644
--- a/spec/std/socket/socket_spec.cr
+++ b/spec/std/socket/socket_spec.cr
@@ -171,7 +171,7 @@ describe Socket, tags: "network" do
   {% end %}
 
   describe "#finalize" do
-    it "flushes" do
+    it "does not flush" do
       port = unused_local_port
       server = Socket.tcp(Socket::Family::INET)
       server.bind("127.0.0.1", port)
@@ -190,7 +190,8 @@ describe Socket, tags: "network" do
 
       socket = Socket.tcp(Socket::Family::INET)
       socket.connect(Socket::IPAddress.new("127.0.0.1", port))
-      socket.gets.should eq "foobar"
+
+      socket.gets.should eq "foo"
     ensure
       socket.try &.close
       server.try &.close
diff --git a/src/crystal/system/unix/file_descriptor.cr b/src/crystal/system/unix/file_descriptor.cr
index 0c3ece9cfff8..d235114849b4 100644
--- a/src/crystal/system/unix/file_descriptor.cr
+++ b/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
diff --git a/src/crystal/system/unix/socket.cr b/src/crystal/system/unix/socket.cr
index 33ac70659b9f..7c39e140849c 100644
--- a/src/crystal/system/unix/socket.cr
+++ b/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*)
diff --git a/src/crystal/system/win32/file_descriptor.cr b/src/crystal/system/win32/file_descriptor.cr
index dc8d479532be..eb2ca700743e 100644
--- a/src/crystal/system/win32/file_descriptor.cr
+++ b/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
diff --git a/src/crystal/system/win32/socket.cr b/src/crystal/system/win32/socket.cr
index 17e4ca875dbb..4f4e87fe835e 100644
--- a/src/crystal/system/win32/socket.cr
+++ b/src/crystal/system/win32/socket.cr
@@ -375,11 +375,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 system_close
+    system_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*)
diff --git a/src/io/file_descriptor.cr b/src/io/file_descriptor.cr
index d4459e9bbe0c..e98a3007d5c2 100644
--- a/src/io/file_descriptor.cr
+++ b/src/io/file_descriptor.cr
@@ -236,7 +236,7 @@ class IO::FileDescriptor < IO
   def finalize
     return if closed? || !close_on_finalize?
 
-    close rescue nil
+    file_descriptor_close { } # ignore error
   end
 
   def closed? : Bool
diff --git a/src/socket.cr b/src/socket.cr
index ca484c0140cc..596aa65b19d5 100644
--- a/src/socket.cr
+++ b/src/socket.cr
@@ -422,7 +422,7 @@ class Socket < IO
   def finalize
     return if closed?
 
-    close rescue nil
+    socket_close { } # ignore error
   end
 
   def closed? : Bool

From 07949f17293914b81da4bbe2c637b29da4218ff1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Johannes=20M=C3=BCller?= <straightshoota@gmail.com>
Date: Thu, 8 Aug 2024 21:31:36 +0200
Subject: [PATCH 4/7] fixup

---
 src/crystal/system/win32/socket.cr | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/crystal/system/win32/socket.cr b/src/crystal/system/win32/socket.cr
index 4f4e87fe835e..d1bbe095a874 100644
--- a/src/crystal/system/win32/socket.cr
+++ b/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)

From ee0bcd346746587e89b24276b7e373c6b5e73a49 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Johannes=20M=C3=BCller?= <straightshoota@gmail.com>
Date: Thu, 8 Aug 2024 22:43:45 +0200
Subject: [PATCH 5/7] fixup

---
 src/crystal/system/win32/socket.cr | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/crystal/system/win32/socket.cr b/src/crystal/system/win32/socket.cr
index d1bbe095a874..78645d51f320 100644
--- a/src/crystal/system/win32/socket.cr
+++ b/src/crystal/system/win32/socket.cr
@@ -384,8 +384,8 @@ module Crystal::System::Socket
     end
   end
 
-  def system_close
-    system_close do |err|
+  def socket_close
+    socket_close do |err|
       raise ::Socket::Error.from_os_error("Error closing socket", err)
     end
   end

From 5f331002a06b4407e7d52ee4decc2daff2de3389 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Johannes=20M=C3=BCller?= <straightshoota@gmail.com>
Date: Fri, 9 Aug 2024 11:16:43 +0200
Subject: [PATCH 6/7] Add docs

---
 src/io/file_descriptor.cr | 11 +++++++++++
 src/socket.cr             |  8 ++++++++
 2 files changed, 19 insertions(+)

diff --git a/src/io/file_descriptor.cr b/src/io/file_descriptor.cr
index e98a3007d5c2..8940a118041f 100644
--- a/src/io/file_descriptor.cr
+++ b/src/io/file_descriptor.cr
@@ -233,6 +233,17 @@ 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?
 
diff --git a/src/socket.cr b/src/socket.cr
index 596aa65b19d5..1d367f805343 100644
--- a/src/socket.cr
+++ b/src/socket.cr
@@ -419,6 +419,14 @@ 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?
 

From dc71a5605a40d10b6a86f67f3b803d135109ba31 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Johannes=20M=C3=BCller?= <straightshoota@gmail.com>
Date: Fri, 9 Aug 2024 11:28:20 +0200
Subject: [PATCH 7/7] Add internal docs

---
 src/crystal/system/file.cr            | 7 -------
 src/crystal/system/file_descriptor.cr | 8 ++++++++
 src/crystal/system/socket.cr          | 8 ++++++++
 3 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/src/crystal/system/file.cr b/src/crystal/system/file.cr
index 75985c107fd5..452bfb6e4ead 100644
--- a/src/crystal/system/file.cr
+++ b/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) %}
diff --git a/src/crystal/system/file_descriptor.cr b/src/crystal/system/file_descriptor.cr
index 0180627d59ce..0652ed56e52a 100644
--- a/src/crystal/system/file_descriptor.cr
+++ b/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
diff --git a/src/crystal/system/socket.cr b/src/crystal/system/socket.cr
index 2669b4c57bca..10f902e9f0c1 100644
--- a/src/crystal/system/socket.cr
+++ b/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