Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix race conditions in Faraday::Utils::Headers #250

Merged
merged 1 commit into from
Apr 1, 2013

Conversation

leocassarani
Copy link
Contributor

Faraday::Utils::Headers uses a shared KeyMap hash to cache symbol -> string mappings of HTTP header names. When multiple threads are writing to the cache, race conditions may occur, as the Hash class is not thread-safe in Ruby.

This commit adds a mutex to synchronize writes to the KeyMap hash. This was enough to remove the race conditions I was seeing in my multi-threaded app running on Rubinius.

@mislav
Copy link
Contributor

mislav commented Apr 1, 2013

Good catch. Two things:

  1. There's no reason to assign the mutex to a constant. It can simply be a local variable declared above the hash block; it will be accessible inside the block just fine.
  2. Mutex is not guaranteed to be defined before you require 'thread'. Utils file should do the require

@coveralls
Copy link

Coverage decreased (-0.16%) when pulling 9719f8d on leocassarani:keymap-mutex into ca8c169 on lostisland:master.

View Details

@leocassarani
Copy link
Contributor Author

Hey @mislav, thanks for the feedback.

  1. You're absolutely right. I was quite conflicted as to what I should assign the mutex to. My main reason for assigning it to a constant was that it seemed a bit more readable and I could declare it after KeyMap itself. But I'm very happy to move it to a local variable and define it above it so the closure can "capture" it.
  2. Very good point (and one I always forget). I'll amend the commit.

Faraday::Utils::Headers uses a shared 'KeyMap' hash to cache
symbol -> string mappings of HTTP header names. When multiple threads
are writing to the cache, race conditions may occur, as the Hash class
is not thread-safe in Ruby.

This commit adds a mutex to synchronize writes to the KeyMap hash.
@coveralls
Copy link

Coverage increased (+0.08%) when pulling 69096c0 on leocassarani:keymap-mutex into ca8c169 on lostisland:master.

View Details

sferik added a commit that referenced this pull request Apr 1, 2013
Fix race conditions in Faraday::Utils::Headers
@sferik sferik merged commit c395b0c into lostisland:master Apr 1, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants