Permalink
Browse files

make sure log file is written in binary mode. fixes #497

  • Loading branch information...
1 parent a2b1be2 commit f2c6f04f3b1b55c6516cb9376f109dcaa213db5d @tenderlove tenderlove committed May 10, 2011
Showing with 41 additions and 0 deletions.
  1. +2 −0 activesupport/lib/active_support/buffered_logger.rb
  2. +39 −0 activesupport/test/buffered_logger_test.rb
@@ -49,10 +49,12 @@ def initialize(log, level = DEBUG)
@log = log
elsif File.exist?(log)
@log = open(log, (File::WRONLY | File::APPEND))
+ @log.binmode
@log.sync = true
else
FileUtils.mkdir_p(File.dirname(log))
@log = open(log, (File::WRONLY | File::APPEND | File::CREAT))
+ @log.binmode
@log.sync = true
end
end
@@ -2,6 +2,7 @@
require 'multibyte_test_helpers'
require 'stringio'
require 'fileutils'
+require 'tempfile'
require 'active_support/buffered_logger'
class BufferedLoggerTest < Test::Unit::TestCase
@@ -16,6 +17,44 @@ def setup
@logger = Logger.new(@output)
end
+ def test_write_binary_data_to_existing_file
+ t = Tempfile.new ['development', 'log']
+ t.binmode
+ t.write 'hi mom!'
+ t.close
+
+ logger = Logger.new t.path
+ logger.level = Logger::DEBUG
+
+ str = "\x80"
+ if str.respond_to?(:force_encoding)
+ str.force_encoding("ASCII-8BIT")
+ end
+
+ logger.add Logger::DEBUG, str
+ logger.flush
+ ensure
+ logger.close
+ t.close true
+ end
+
+ def test_write_binary_data_create_file
+ fname = File.join Dir.tmpdir, 'lol', 'rofl.log'
+ logger = Logger.new fname
+ logger.level = Logger::DEBUG
+
+ str = "\x80"
+ if str.respond_to?(:force_encoding)
+ str.force_encoding("ASCII-8BIT")
+ end
+
+ logger.add Logger::DEBUG, str
+ logger.flush
+ ensure
+ logger.close
+ File.unlink fname
+ end
+
def test_should_log_debugging_message_when_debugging
@logger.level = Logger::DEBUG
@logger.add(Logger::DEBUG, @message)

11 comments on commit f2c6f04

Owner

fxn replied May 16, 2011

binmode disables newline conversion, which is going to be really bad for Windows users.

Owner

tenderlove replied May 16, 2011

@fxn can you provide a test that exercises that problem? If we don't use binmode, we will have encoding conversion errors (which these these will show).

Owner

fxn replied May 16, 2011

@tenderlove yes saw the ticket, but this fix seems too expensive, it breaks portable newlines for writing to the log. You know, the C stdlib I/O layer in MRI won't do LF -> CRLF conversion, and thus log files produced on Windows computers will appear as one long single line there. Same happens in other interpreters because they imitate MRI even if the underlying platform do not follow the C way to handle portable newlines (eg JRuby).

Owner

tenderlove replied May 17, 2011

@luislavena care to comment on this?

Hello,

@fxn, the only edtior that is going to be affected by this is Notepad, and see that I remarked editor

Console tools, even type will properly process single \n without the return carrier (\r)

So @tenderlove, I'm Luis Lavena, and I approve this commit.

Owner

fxn replied May 18, 2011

@luislavena but in my view Rails is not a good citizen on Windows writing newlines in text files that do not follow the platform convention. Even if some editors may display correctly a file with CRLF on Unix, if Rails writed CRLFs one would say WTF is this?

A portable program writes portable newlines, that's my point.

So, if there was a solution that outputs portable newlines and only works with the encodings, that would be better in my view.

Owner

fxn replied May 18, 2011

Let me add that most programs and scripts will get the lines correctly because albeit the transformation CRLF -> LF that most I/O libraries perform when reading is not happening, your line-oriented program will see the LFs anyway (when working with text the CR never comes up to your program string buffers on Windows working in text mode).

But as I said before I am not concerned about particular uses of the log, my concern is about using a convention that is not the one of the runtime platform. That's not good. Good programs are portable. Writing non-native newlines is a broken window that will show up somewhere.

Owner

fxn replied May 18, 2011

This is what I mean 9d8e2fb.

Set a binary encoding, but work in text mode to keep Rails portable.

@fxn, if you wanted the log be friendly with the platform, then you would strip ANSI coloring in the first place, since Windows can't handle ANSI coloring by default.

Even if you have CRLF, the log files still have these non ASCII characters that do not display properly in the platform.

Is that been taken care? I believe not, I still see ANSI escape codes in my log files, which are not printable characters, notepad makes a mess and user still gets annoyed by them.

So, you're compromising the performance of something against the half portability of something that is not doing the right thing.

Doing what you're doing in 9d8e2fb might not have the desired results under certain encoding configurations of the user.

But, heck, seems you have better idea of encodings :-)

Owner

fxn replied May 18, 2011

@luislavena I am not addressing all portability issues in Rails in this thread. Not sure what ANSI escape sequences have to do here.

All I am saying is that if we can fix that ticket without compromising portable newlines, that solution is better. Do you really find this point of view strange?

If the new patch breaks something, it is not certainly the test case for that ticket. If the new patch breaks some particular combination of things I'd really want to know, and still find a solution that uses text mode if possible.

Owner

fxn replied May 18, 2011

Regarding whether ANSI has been taken care of, there's a flag config.colorize_logging. And if that didn't exist, it should be fixed anyway. In my view portability is a goal for programs that are going to run in several platforms. If possible.

I really find amazing that the reaction to the comments above is not "oh of course that was an undesired side-effect, let's see whether we can fix and still write in text mode somehow". I really need to defend that being portable is desirable if possible?

Please sign in to comment.