Rubygems does not terminate on failed file lock when not superuser

[duckinator]

Description:

Supersedes PR #1536.

Fixes issue #1535.

Tasks:

• Describe the problem / feature
• Write tests
• Write code to solve the problem
• Get code review from coworkers / friends
• Squash commits

I will abide by the code of conduct.

[duckinator]

As a note for anyone looking at this, this still needs tests.

[duckinator]

Looking at the error Travis CI returns: Line 841 is a comment. I have no idea what it's doing.

I am completely lost as to what the hell Appveyor is doing.

[segiddins]

According to GH, line 841 is rescue *READ_BINARY_ERRORS

[duckinator]

Okay, something was up locally. That's what I'm seeing locally now, too.

As for the actual issue, it looks like READ_BINARY_ERRORS is nil?

irb(main):001:0>   READ_BINARY_ERRORS = begin
irb(main):002:1*     read_binary_errors = [Errno::EACCES]
<inary_errors << Errno::ENOTSUP if Errno.const_defined?(:ENOTSUP)
irb(main):004:1>   end.freeze
=> nil
irb(main):005:0> READ_BINARY_ERRORS
=> nil
irb(main):006:0>

[duckinator]

Oh! I see what the issue is. I need to return read_binary_errors right before the end.freeze, or it'll return nil on 1.8.7.

[duckinator]

Well, that's Travis errors fixed. Appveyor failure is caused by #1586.

[segiddins]

this will fail on 1.8.7 as well, no?

[duckinator]

looks like it, yeah.

[duckinator]

@segiddins the issue I'm running into here is since it only has one class, if I use the approach that was used for Gem.read_binary, then on 1.8.7 it'll be equivalent to this:

begin
  io.flock(File::LOCK_EX)
rescue
end

which seems... bad?

Any idea how to approach this?

[duckinator]

Rebased, since Appveyor builds have been fixed on master.

[duckinator]

@segiddins do you have any thoughts on how to approach testing this? I'm having trouble wrapping my head around what exactly needs testing; it seems like 2-3 things need tests for both read_binary and write_binary, but I'm not quite sure what they are.

[segiddins]

couldn't ENOTSUP be undefined here as well?

[duckinator]

Fixed.

[segiddins]

this looks good, other than the failing specs

[duckinator]

Behold, the mighty question mark, breaker of tests.

(I have to begrudgingly admit I committed without running the tests locally...)

[duckinator]

@segiddins specs are fixed, btw.

[segiddins]

@HoMr r+
Thanks @duckinator !

[duckinator]

@segiddins you @'d @HoMr instead of @homu.

[segiddins]

@homu r+
😬

[homu]

📌 Commit 23d8e9c has been approved by segiddins

[homu]

⌛ Testing commit 23d8e9c with merge 6c6d268...

[homu]

☀️ Test successful - status