Fix CI #224

Merged
merged 15 commits into from Dec 28, 2012

Projects

None yet

3 participants

@technoweenie
Member

Saw some weird errors from Travis CI on master. I backed up to the last working commit and made the 0.9 branch. I merged some patches in, tracked the error down to an autoload related thing, and fixed it. Now I'm applying those patches to master, hoping it's fixed there too.

Also bringing #223 over for the ride too.

@technoweenie technoweenie merged commit b25adc0 into master Dec 28, 2012

1 check passed

default The Travis build passed
Details
@technoweenie technoweenie deleted the fix-ci branch Dec 28, 2012
@eric eric commented on the diff Dec 28, 2012
lib/faraday.rb
@@ -181,7 +181,12 @@ def lookup_middleware(key)
end
def middleware_mutex(&block)
- (@middleware_mutex ||= Mutex.new).synchronize(&block)
+ @middleware_mutex ||= Mutex.new
@eric
eric Dec 28, 2012 Contributor

This seems like a very dubious way to initialize a Mutex. If you have two threads that call middleware_mutex at the same time, they may get different mutexes the first time.

Also, instead of trying to use @middleware_mutex.locked? to create a poor mans recursive mutex, this should really just use Monitor instead. It acts just like a mutex but it's recursive.

@technoweenie
technoweenie Dec 28, 2012 Member

Yea, I have no idea what I'm doing here. Just trying to remove autoload.
It'd be awesome if someone that wanted autoload gone would help remove it
:)

It looks like I can essentially replace Mutex with Monitor, and it'll
magically work?

On Fri, Dec 28, 2012 at 1:54 AM, Eric Lindvall notifications@github.comwrote:

In lib/faraday.rb:

@@ -181,7 +181,12 @@ def lookup_middleware(key)
end

 def middleware_mutex(&block)
  •  (@middleware_mutex ||= Mutex.new).synchronize(&block)
    
  •  @middleware_mutex ||= Mutex.new
    

This seems like a very dubious way to initialize a Mutex. If you have two
threads that call middleware_mutex at the same time, they may get
different mutexes the first time.

Also, instead of trying to use @middleware_mutex.locked? to create a poor
mans recursive mutex, this should really just use Monitor instead. It
acts just like a mutex but it's recursive.


Reply to this email directly or view it on GitHubhttps://github.com/technoweenie/faraday/pull/224/files#r2514046.

Rick Olson
http://github.com/technoweenie

@eric
eric Dec 28, 2012 Contributor

That's correct. Monitor has the same interface.

@technoweenie
technoweenie Dec 28, 2012 Member

Rad, thanks.

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