[thread] [rfc] patch to change condition_variable and mutex error checks
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Patch is attached. I'd like to get some comments about this patch. Does it seem reasonable enough to accept? Is there a better way to handle it? BOOST_VERIFY aborts on EINTR, but EINTR is usually not fatal, it just means you need to try later. So, the attached patch makes it so that the condition_variable and mutex error checks retry on EINTR, but will still fail on any other error. This issue was uncovered on one of Phusion Passenger's customers' production systems. - -- - ---Brett. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iQEcBAEBAgAGBQJO1pVQAAoJEEAzW/nB31+3CgMIAMPYRMvuuYWdHbmWF7qWt13i pKRLGDjT5fT75SzVdcmwFNJLlboaP5tNi8g89Kw+iS+jjghEbyZr2Zx6BdCL67ne +XtA1DqlmNHD3GZiLMlQDdPqbV7qliqj4d7+8xXqXnivxULYPwQMzW1XXY8Cs4Ys uHBK/NrvZM+/LL2umHSCOLbKz7AEli6tSzuUyw557ubp42UPnE+gc+mUb7QYmcgW BKzizZ03s+V4qJA/B+N4Ux8RvvhWgNLXnYoC958mzvnTwaTbdGfEmzVkKaWucI50 jsEzfejn/Gru2BNeC2MwVmecxmkHugoXxvVUYIH4PaDET96BM3ihHWV+4SLg7ik= =xkyz -----END PGP SIGNATURE-----
Brett Lentz wrote:
Patch is attached.
I'd like to get some comments about this patch. Does it seem reasonable enough to accept? Is there a better way to handle it?
BOOST_VERIFY aborts on EINTR, but EINTR is usually not fatal, it just means you need to try later.
You've attached the wrong patch. Anyway, POSIX specifically forbids pthread functions from returning EINTR.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 11/30/2011 04:51 PM, Peter Dimov wrote:
Brett Lentz wrote:
Patch is attached.
I'd like to get some comments about this patch. Does it seem reasonable enough to accept? Is there a better way to handle it?
BOOST_VERIFY aborts on EINTR, but EINTR is usually not fatal, it just means you need to try later.
You've attached the wrong patch. Anyway, POSIX specifically forbids pthread functions from returning EINTR.
DOH! Correct patch attached here. Posix may forbid it, but that doesn't necessarily stop a broken implementation from doing it. Like the original e-mail said, this was a result of an actual customer's experience in an actual production situation. While I'd like to see the patch accepted, it would be equally valid to just say that it's not a problem boost should be solving, and that I need to file a bug with the OS vendor that they shouldn't be violating POSIX like this. ;-) - -- - ---Brett. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iQEcBAEBAgAGBQJO1qioAAoJEEAzW/nB31+3kjYH/3pCgE37HZFr/WB6uW9QhQrS Iip2xPBbW7AbwP5Xe24AhBwYwK//htiAhpJrrkYayRjrbhCmFkaax2/CCbiYte0B YFrqa3NJh1KqSm4ulBFCr9Pw3PGolcN5ck2j4X/wlDHCdQWQAE1dOPoK4v8Wnn/5 LqjFMpbrXCVcE4dB4BrYGyZXRKnNFs0eBAW8bE2bHgZK57ES1GAJpGJQ4FY2ebpp 4XU862D/eN7s7A2fgcp9llQ9ooaz2iAkip3HIQWG1fuTNrsdTw8xD0rBUHNMhzoQ mfjHB9QqvtIrYtMFJS4j1LkkoOXhvHlcc7+yIvnUByEPK59STWloyXg1yE2N964= =1esn -----END PGP SIGNATURE-----
Le 30/11/11 23:05, Brett Lentz a écrit :
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 11/30/2011 04:51 PM, Peter Dimov wrote:
Brett Lentz wrote:
Patch is attached.
I'd like to get some comments about this patch. Does it seem reasonable enough to accept? Is there a better way to handle it?
BOOST_VERIFY aborts on EINTR, but EINTR is usually not fatal, it just means you need to try later. You've attached the wrong patch. Anyway, POSIX specifically forbids pthread functions from returning EINTR.
I wasn't aware of this. DOH! Correct patch attached here.
Posix may forbid it, but that doesn't necessarily stop a broken implementation from doing it. Like the original e-mail said, this was a result of an actual customer's experience in an actual production situation. I agree that this should be managed by Boost.Thread as other workarounds. As many others we have been forced to use this idiom since a long time. Even if the patch is not too much time consuming it could use conditional compilation and be included only on bad posix implementations. While I'd like to see the patch accepted, it would be equally valid to just say that it's not a problem boost should be solving, and that I need to file a bug with the OS vendor that they shouldn't be violating POSIX like this. ;-)
Yes, please. Best, Vicente
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 11/30/2011 06:21 PM, Vicente J. Botet Escriba wrote:
On 11/30/2011 04:51 PM, Peter Dimov wrote:
Brett Lentz wrote:
Patch is attached.
I'd like to get some comments about this patch. Does it seem reasonable enough to accept? Is there a better way to handle it?
BOOST_VERIFY aborts on EINTR, but EINTR is usually not fatal, it just means you need to try later. You've attached the wrong patch. Anyway, POSIX specifically forbids pthread functions from returning EINTR.
I wasn't aware of this. DOH! Correct patch attached here.
Posix may forbid it, but that doesn't necessarily stop a broken implementation from doing it. Like the original e-mail said, this was a result of an actual customer's experience in an actual production situation. I agree that this should be managed by Boost.Thread as other workarounds. As many others we have been forced to use this idiom since a long time. Even if the patch is not too much time consuming it could use conditional compilation and be included only on bad
Le 30/11/11 23:05, Brett Lentz a écrit : posix implementations.
While I'd like to see the patch accepted, it would be equally valid to just say that it's not a problem boost should be solving, and that I need to file a bug with the OS vendor that they shouldn't be violating POSIX like this. ;-)
Yes, please.
Best, Vicente
I've created ticket #6200 for this patch. I'm going to be investigating the upstream pthread library to see if I can find out whether they've addressed the issue. However, I do think that this patch has some value in protecting downstream consumers of Boost from a broken threading library. This means that any downstream consumer's effort is spent dealing with something other than a known (POSIX violating) bug. To me, that's a win. - -- - ---Brett. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iQEcBAEBAgAGBQJO2PTCAAoJEEAzW/nB31+3L/EIAOwU2n7g+/VZlsNJFYX25VvI 0xc3mlCXTGBBuRBqdCe2JSRbcc47069EW3qDt2Mn5T1x8oUkoGoOe+aEGlzgdVE6 YYCwax0NBjnk4BVUvq1LGyM1PLAd4t47Lf2F0mxQ8miC058Jj3k3/DQLyYHUxwr5 J4GQpkifz3cOY+0JEgNhvsNo577EUNrcbDwdRukIUkKjreayFeL6O+u5JBCAagio 9CRfPz4C4E102UIOijIQHslO4mW2+dp15Ay7+iKf0+6luXeAd/oQokQadH8HQ5Vl hI2rrYiS4GgXZhTLHf5v//ISKAPPRny18Fc1UXq7A5mIlxwibgURSxg3Xycqq3E= =affp -----END PGP SIGNATURE-----
Le 02/12/11 16:54, Brett Lentz a écrit :
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 11/30/2011 06:21 PM, Vicente J. Botet Escriba wrote:
On 11/30/2011 04:51 PM, Peter Dimov wrote:
Brett Lentz wrote:
Patch is attached.
I'd like to get some comments about this patch. Does it seem reasonable enough to accept? Is there a better way to handle it?
BOOST_VERIFY aborts on EINTR, but EINTR is usually not fatal, it just means you need to try later. You've attached the wrong patch. Anyway, POSIX specifically forbids pthread functions from returning EINTR.
I wasn't aware of this. DOH! Correct patch attached here.
Posix may forbid it, but that doesn't necessarily stop a broken implementation from doing it. Like the original e-mail said, this was a result of an actual customer's experience in an actual production situation. I agree that this should be managed by Boost.Thread as other workarounds. As many others we have been forced to use this idiom since a long time. Even if the patch is not too much time consuming it could use conditional compilation and be included only on bad
Le 30/11/11 23:05, Brett Lentz a écrit : posix implementations.
While I'd like to see the patch accepted, it would be equally valid to just say that it's not a problem boost should be solving, and that I need to file a bug with the OS vendor that they shouldn't be violating POSIX like this. ;-) Yes, please.
Best, Vicente
I've created ticket #6200 for this patch.
This part is not correct 55 res=pthread_cond_wait(&cond,&internal_mutex); 55 int ret; 56 do { 57 ret = pthread_cond_wait(&cond,m.mutex()->native_handle()); 58 } while (ret == EINTR); Note that before the mutex used was internal_mutex. Are you sure that this patch is working for your applications? https://svn.boost.org/trac/boost/ticket/6200#comment:1 Best, Vicente
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 12/02/2011 03:21 PM, Vicente J. Botet Escriba wrote:
Le 02/12/11 16:54, Brett Lentz a écrit :
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 11/30/2011 06:21 PM, Vicente J. Botet Escriba wrote:
Le 30/11/11 23:05, Brett Lentz a écrit :
On 11/30/2011 04:51 PM, Peter Dimov wrote:
Brett Lentz wrote:
Patch is attached.
I'd like to get some comments about this patch. Does it seem reasonable enough to accept? Is there a better way to handle it?
BOOST_VERIFY aborts on EINTR, but EINTR is usually not fatal, it just means you need to try later. You've attached the wrong patch. Anyway, POSIX specifically forbids pthread functions from returning EINTR.
I wasn't aware of this. DOH! Correct patch attached here.
Posix may forbid it, but that doesn't necessarily stop a broken implementation from doing it. Like the original e-mail said, this was a result of an actual customer's experience in an actual production situation. I agree that this should be managed by Boost.Thread as other workarounds. As many others we have been forced to use this idiom since a long time. Even if the patch is not too much time consuming it could use conditional compilation and be included only on bad posix implementations. While I'd like to see the patch accepted, it would be equally valid to just say that it's not a problem boost should be solving, and that I need to file a bug with the OS vendor that they shouldn't be violating POSIX like this. ;-) Yes, please.
Best, Vicente
I've created ticket #6200 for this patch.
This part is not correct
55 res=pthread_cond_wait(&cond,&internal_mutex);
55 int ret; 56 do { 57 ret = pthread_cond_wait(&cond,m.mutex()->native_handle()); 58 } while (ret == EINTR);
Note that before the mutex used was internal_mutex. Are you sure that this patch is working for your applications?
https://svn.boost.org/trac/boost/ticket/6200#comment:1
Best, Vicente
You're absolutely right. This section has changed between the forked version I'm working from (1.44) and the current development trunk (1.49). Looks like I need to redo the patch. - -- - ---Brett. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iQEcBAEBAgAGBQJO2U6BAAoJEEAzW/nB31+37JYIAIYRN3JJwxWBFVgBeTGT/lPr 9PXE//+xII6D/FPpvvP16Bi75LYOH7HB2s8v6c2araq32M6RRO4AgQN6eHgRkW9F CMjJ82pgh5Bx0CQGL5/6NQevfozWBfHHmBjp/cgf/Jglc7e6gYkLQG3ImYGdzO6K 0vT2V35WVWDbGq1k6FDpM3/Y0baLF9R3VAogoZ6kW6GwvRA1aWFUVW4HzSOF4lne 0UzzjsVsLYwEre6WHCn6/T1MbKeWRGQhATTy9ZFbFdW8KBPFbe65M+rdMflyYHU/ t8k99vPCF8v6PX/ONf24W4vKiK5Er4lpKL+m4U8BDbw3Jk+a6/AsFA9AYOfcp/8= =VfGl -----END PGP SIGNATURE-----
participants (3)
-
Brett Lentz -
Peter Dimov -
Vicente J. Botet Escriba