Set IO to be sync for File by default

[jgaskins]

Since userspace buffers prevent Crystal from closing files automatically on program exit, maybe we should avoid using them for file I/O by default. Folks can enable userspace buffering explicitly if they need its benefits.

Fixes #13995

[yxhuvud]

Somewhat meta: The wording around this is so bad. Why on earth is the base verb used sync and not buffer or even write_buffer?

That said I agree with the basic premise of matching the promises of the underlying OS by default, or at least to not be less safe than the OS. That way people will hopefully get less bad surprises even if things are too complicated in general: https://transactional.blog/how-to-learn/disk-io

Also with things like
https://lore.kernel.org/linux-fsdevel/[email protected]/t/#u on the horizon getting rid of complexity by default might make even more sense. But having easy access to buffering as an opt in option do still make a lot of sense (that option might also make sense for the buffered data, to avoid double buffering).

[jgaskins]

Why on earth is the base verb used sync and not buffer or even write_buffer?

I've been wondering this for a while, too. I went to open an issue about it but someone already had. #9023 😄

[jgaskins]

Also, the spec failures appear to be due to what @straight-shoota mentioned here. I think those specs should change, even if just to explicitly set sync = false, because they depend on File defaulting to it.

[RX14]

Why should files behave differently to i.e. sockets here? If you call exit at a random point (i believe this is the only case that makes a difference) why would you expect all your file IO to complete but not your socket IO?

[yxhuvud]

If you call exit

There is no difference from program terminating normally, no?

[jgaskins]

Why should files behave differently to i.e. sockets here?

I don't understand the question. They already behave differently, and this PR aligns them better. To illustrate what I mean, this program does not write the string to the file:

file = File.open("output.txt", "w")
file.puts "this is some output"
sleep 100.milliseconds

This program sends data over a socket, reads from the other end of the socket, and writes to STDOUT what the TCP client sent:

require "socket"

spawn do
  server = TCPServer.new(1234)
  while incoming = server.accept?
    spawn puts incoming.gets
  end
end

socket = TCPSocket.new("localhost", 1234)
socket.puts "hi"

# Let the other fibers work
sleep 100.milliseconds

The first one doesn't need the sleep call since file I/O is blocking (side note: blocking vs sync is only a useful distinction to the tiniest subset of programmers — the terms are frequently used interchangeably), but I put it in there to show I had nothing up my sleeves. Neither program flushes or closes any IO objects before exiting.

The second program outputs to STDOUT. The first does not write to the file. And that doesn't make any sense.

[HertzDevil]

If this is a concern, why would non-blocking, regular files still have buffered writes?

[jgaskins]

If this is a concern, why would non-blocking, regular files still have buffered writes?

Files are blocking by default, aren't they?

crystal/src/io/file_descriptor.cr

Lines 52 to 58 in b87d3e8

	blocking =
	case system_info.type
	when .pipe?, .socket?, .character_device?
	false
	else
	true
	end

[HertzDevil]

Well let's say I indeed passed blocking: false explicitly when opening a File. This PR suggests that it is okay for those files not to be flushed on program exit.

[jgaskins]

This PR suggests that it is okay for those files not to be flushed on program exit.

I'm very verbose in my writing. If I didn't write something, chances are I wasn't suggesting it, either.

What I did say in the very first sentence of this PR was that this is about the default behavior. The default settings are how people will use these most of the time, especially people who are less familiar with the language.

At the moment, I don't have many opinions for when people override the default settings, tbh.

[HertzDevil]

Disabling write buffering when File.new is called with 1 out of 5 optional arguments being blocking: true is a default behavior. Disabling write buffering in File.new regardless of blocking's value is also a defualt behavior.

Why do you think the former is more appropriate than the latter?

[jgaskins]

Disabling write buffering in File.new regardless of blocking's value is also a defualt behavior.

If this were true, wouldn't the File example in this comment write content to the file? Because that's not what's happening.

All I'm looking for is for Crystal to do the safe thing by default. Nothing more than that. I don't understand why there is so much pushback.

[HertzDevil]

That example has nothing to do with my question. Maybe let me put it this way:

class File
  private def initialize(@path, fd : Int, blocking = false, encoding = nil, invalid = nil)
    self.set_encoding(encoding, invalid: invalid) if encoding
    super(fd, blocking)
  end
end

If I add self.sync = false to this constructor, that also disables write buffering on all Files by default. So why do you think passing blocking: false to File (not IO::FileDescriptor) does not count as part of this default?

[jgaskins]

I can't tell for sure, but I feel like your questions are leading toward pointing out that you would prefer to split out blocking and sync as separate options. Is that where you're going with this?

[RX14]

They already behave differently, and this PR aligns them better

My apologies, I thought Socket was sync = false by default. Perhaps a confusion caused by HTTP::Server::Response being sync = true by default.

[jgaskins]

My apologies, I thought Socket was sync = false by default.

No worries, when you asked I thought I might've been the one that was mistaken. I wrote up the example code because I didn't want to be an IRL Principal Skinner meme.

[crysbot]

This pull request has been mentioned on Crystal Forum. There might be relevant details there:

https://forum.crystal-lang.org/t/using-wait-groups-with-http-server/7452/4

[straight-shoota]

I'm sympathetic to disable output buffering for (ordinary) files by default. But I don't think this PR is the best way to do it.
First of all, this configuration should be considered a cross-platform default, so I'd prefer not to have it in platform-specific code.
Also with this change most file descriptors will be unbuffered by default, so it would seem a good idea to disable it globally in IO::FileDescriptor and only enable it explicitly for the few cases which should still use output buffering (if there are any).

Before jumping to a new implementation, I believe this topic may need a bit more discussion. Changing such a default behaviour must be explored deeply to understand its effects. Especially since this affects the conceptual premise of streams in Crystal (this is already the case for sockets of course, but files makes it even more an issue).