Skip to content

Commit

Permalink
Avoid flush in Socket and IO::FileDescriptor finalizer
Browse files Browse the repository at this point in the history
  • Loading branch information
straight-shoota committed Aug 8, 2024
1 parent 69345f2 commit 668de3b
Show file tree
Hide file tree
Showing 8 changed files with 39 additions and 10 deletions.
4 changes: 2 additions & 2 deletions spec/std/io/file_descriptor_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -64,15 +64,15 @@ 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"
file.flush
file << "bar"
file.finalize

File.read(path).should eq "foobar"
File.read(path).should eq "foo"
ensure
file.try(&.close) rescue nil
end
Expand Down
5 changes: 3 additions & 2 deletions spec/std/socket/socket_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
Expand Down
10 changes: 8 additions & 2 deletions src/crystal/system/unix/file_descriptor.cr
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
Expand Down
12 changes: 11 additions & 1 deletion src/crystal/system/unix/socket.cr
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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*)
Expand Down
6 changes: 6 additions & 0 deletions src/crystal/system/win32/file_descriptor.cr
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 7 additions & 1 deletion src/crystal/system/win32/socket.cr
Original file line number Diff line number Diff line change
Expand Up @@ -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*)
Expand Down
2 changes: 1 addition & 1 deletion src/io/file_descriptor.cr
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion src/socket.cr
Original file line number Diff line number Diff line change
Expand Up @@ -422,7 +422,7 @@ class Socket < IO
def finalize
return if closed?

close rescue nil
socket_close { } # ignore error

Check failure on line 425 in src/socket.cr

View workflow job for this annotation

GitHub Actions / x86_64-windows / build

undefined local variable or method 'socket_close' for TCPServer
end

def closed? : Bool
Expand Down

0 comments on commit 668de3b

Please sign in to comment.