Fix ConnectionPool thread-safe issue #1200
Conversation
…mutex to fix thread-safe issue; changed ConnectionPool#clear_stale_cached_connections! to private to avoid outside call, which would not be thread-safe
|
Is it intentional to make the method private? |
|
This can't pass tests. We have tests that call the method you made private. |
|
Tenderlove, please look into my changes, I also changed the test, using send instead. josevalim: it is intentional to make the method private, because the method is not thread-safe called outside of ConnectionPool, all methods called inside ConnectionPool are covered by mutex, so there is no sense to leave the method public |
|
Is the mutex reentrant? If so, would it be feasible to synchronize Using |
|
Hi, since there are some concerns about making |
Certain groups of web proxies do not set these values properly. Notably, proxies for cell phones, which often do not set the remote IP information correctly (not surprisingly, since the clients do not have an IP address). Allowing this to be configurable makes it possible for developers to choose to ignore this simple spoofing check, when a significant amount of their traffic would result in false positives anyway. Signed-off-by: Michael Koziarski <michael@koziarski.com> [rails#1200 state:committed]
|
Do you mind squashed those commits together and force push to your branch? @tenderlove bro, does the change looks ok? |
|
Ya, this is fine. I'll merge it. |
|
@tenderlove can I close this one? |
|
Ya, we can close this. Those methods are synchronized on master now. |
This is a new fix NZKoz suggested, details: #563
Changes made: covered ConnectionPool#connection and #release_connection methods by mutex to fix thread-safe issue; changed ConnectionPool#clear_stale_cached_connections! to private to avoid outside call, which would not be thread-safe,
The old patch with the test provided to prove the thread-safe issue on jruby can be found at: xli@23828c0