File.open w/ File::RDWR should write \r\n but gets should read as \n after rewind on windows #3738

Closed
enebo opened this Issue Mar 16, 2016 · 3 comments

Comments

Projects
None yet
1 participant
@enebo
Member

enebo commented Mar 16, 2016

Environment

Broken in different ways on JRuby 1.7.24 and 9.0.5.0.

Expected Behavior

For the following script:

file = "temptemp"

File.open(file, "w") {}
File.open(file, File::RDWR) do |f|
  f.puts("writing")  # writes "writing\r\n"
  f.flush            # force to disk
  gets               # do not continue until you can verify it was written right
  f.rewind           # backup
  p f.gets           # read it but it should be "writing\n"
end
File.unlink(file)

The comments dictate how it should behave.

Actual Behavior

On JRuby 1.7.24 it will write "writing\r\n" but it will read "writing\r\n". instead of "writing\n".
On JRuby 9.0.5.0 it wil write "writing\n" instead of "writing\r\n" but it will read "writing\n".

@enebo enebo added the windows label Mar 16, 2016

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Mar 16, 2016

Member

Notes since this stuff is scary :)

[Edited for sake of clarity]

In doWriteConversion we end up calling needsWriteConversion which returns true because while open for writing we end up setting TEXTMODE in OpenFile. Because this returns true we end up setting the open file to be binary which in turn ends up not converting the \r\n to \n on the gets. Debugger showed that CRLFStreamWrapper is binary on File::RDWR but not when opened up as a plain "r".

Member

enebo commented Mar 16, 2016

Notes since this stuff is scary :)

[Edited for sake of clarity]

In doWriteConversion we end up calling needsWriteConversion which returns true because while open for writing we end up setting TEXTMODE in OpenFile. Because this returns true we end up setting the open file to be binary which in turn ends up not converting the \r\n to \n on the gets. Debugger showed that CRLFStreamWrapper is binary on File::RDWR but not when opened up as a plain "r".

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Mar 17, 2016

Member

So in looking at MRI a bit I can see the textmode check does not exist if run on windows.

UNIX:

#define NEED_NEWLINE_DECORATOR_ON_WRITE(fptr) ((fptr)->mode & FMODE_TEXTMODE)
#define NEED_WRITECONV(fptr)
(((fptr)->encs.enc != NULL &&
  (fptr)->encs.enc != rb_ascii8bit_encoding()) ||
    NEED_NEWLINE_DECORATOR_ON_WRITE(fptr) ||
    ((fptr)->encs.ecflags & (ECONV_DECORATOR_MASK|ECONV_STATEFUL_DECORATOR_MASK)))

Whereas on Windows:

#define NEED_WRITECONV(fptr)
(((fptr)->encs.enc != NULL &&
  (fptr)->encs.enc != rb_ascii8bit_encoding()) ||
  ((fptr)->encs.ecflags & ((ECONV_DECORATOR_MASK & ~ECONV_CRLF_NEWLINE_DECORATOR)|ECONV_STATEFUL_DECORATOR_MASK)))

So I have found a signifcant discrepency.

Member

enebo commented Mar 17, 2016

So in looking at MRI a bit I can see the textmode check does not exist if run on windows.

UNIX:

#define NEED_NEWLINE_DECORATOR_ON_WRITE(fptr) ((fptr)->mode & FMODE_TEXTMODE)
#define NEED_WRITECONV(fptr)
(((fptr)->encs.enc != NULL &&
  (fptr)->encs.enc != rb_ascii8bit_encoding()) ||
    NEED_NEWLINE_DECORATOR_ON_WRITE(fptr) ||
    ((fptr)->encs.ecflags & (ECONV_DECORATOR_MASK|ECONV_STATEFUL_DECORATOR_MASK)))

Whereas on Windows:

#define NEED_WRITECONV(fptr)
(((fptr)->encs.enc != NULL &&
  (fptr)->encs.enc != rb_ascii8bit_encoding()) ||
  ((fptr)->encs.ecflags & ((ECONV_DECORATOR_MASK & ~ECONV_CRLF_NEWLINE_DECORATOR)|ECONV_STATEFUL_DECORATOR_MASK)))

So I have found a signifcant discrepency.

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Mar 17, 2016

Member

Fixed in commit 4496cff. Not sure why git did not pick this up (perhaps the trailing color after issue number?).

Member

enebo commented Mar 17, 2016

Fixed in commit 4496cff. Not sure why git did not pick this up (perhaps the trailing color after issue number?).

@enebo enebo closed this Mar 17, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment