[JIRA] Created: (LBCORE-67) Unsecure usage of locks in AppenderAttachableImpl
Unsecure usage of locks in AppenderAttachableImpl ------------------------------------------------- Key: LBCORE-67 URL: http://jira.qos.ch/browse/LBCORE-67 Project: logback-core Issue Type: Bug Components: Appender Affects Versions: 0.9.10 Reporter: Joern Huxhorn Assignee: Logback dev list Attachments: LockPatch.patch The unlock of a lock should, I would even say "must", always be done in a finally block. Otherwise really bad things (deadlock) can happen if an exception is thrown. See http://java.sun.com/j2se/1.5.0/docs/api/java/util/concurrent/locks/Lock.html -- This message is automatically generated by JIRA. - If you think it was sent incorrectly contact one of the administrators: http://jira.qos.ch/secure/Administrators.jspa - For more information on JIRA, see: http://www.atlassian.com/software/jira
[ http://jira.qos.ch/browse/LBCORE-67?page=com.atlassian.jira.plugin.system.is... ] Joern Huxhorn updated LBCORE-67: -------------------------------- Attachment: LockPatch.patch Patch fixing the problem.
Unsecure usage of locks in AppenderAttachableImpl -------------------------------------------------
Key: LBCORE-67 URL: http://jira.qos.ch/browse/LBCORE-67 Project: logback-core Issue Type: Bug Components: Appender Affects Versions: 0.9.10 Reporter: Joern Huxhorn Assignee: Logback dev list Attachments: LockPatch.patch
The unlock of a lock should, I would even say "must", always be done in a finally block. Otherwise really bad things (deadlock) can happen if an exception is thrown. See http://java.sun.com/j2se/1.5.0/docs/api/java/util/concurrent/locks/Lock.html
-- This message is automatically generated by JIRA. - If you think it was sent incorrectly contact one of the administrators: http://jira.qos.ch/secure/Administrators.jspa - For more information on JIRA, see: http://www.atlassian.com/software/jira
[ http://jira.qos.ch/browse/LBCORE-67?page=com.atlassian.jira.plugin.system.is... ] Joern Huxhorn commented on LBCORE-67: ------------------------------------- I actually feel quite a bit unsure about using the current version in production because this is more or less a time bomb...
Unsecure usage of locks in AppenderAttachableImpl -------------------------------------------------
Key: LBCORE-67 URL: http://jira.qos.ch/browse/LBCORE-67 Project: logback-core Issue Type: Bug Components: Appender Affects Versions: 0.9.10 Reporter: Joern Huxhorn Assignee: Logback dev list Attachments: LockPatch.patch
The unlock of a lock should, I would even say "must", always be done in a finally block. Otherwise really bad things (deadlock) can happen if an exception is thrown. See http://java.sun.com/j2se/1.5.0/docs/api/java/util/concurrent/locks/Lock.html
-- This message is automatically generated by JIRA. - If you think it was sent incorrectly contact one of the administrators: http://jira.qos.ch/secure/Administrators.jspa - For more information on JIRA, see: http://www.atlassian.com/software/jira
[ http://jira.qos.ch/browse/LBCORE-67?page=com.atlassian.jira.plugin.system.is... ] Ceki Gulcu commented on LBCORE-67: ---------------------------------- Joern, Thank you for expressing your concern. Have you tried creating a test case where the current code would "bomb"?
Unsecure usage of locks in AppenderAttachableImpl -------------------------------------------------
Key: LBCORE-67 URL: http://jira.qos.ch/browse/LBCORE-67 Project: logback-core Issue Type: Bug Components: Appender Affects Versions: 0.9.10 Reporter: Joern Huxhorn Assignee: Logback dev list Attachments: LockPatch.patch
The unlock of a lock should, I would even say "must", always be done in a finally block. Otherwise really bad things (deadlock) can happen if an exception is thrown. See http://java.sun.com/j2se/1.5.0/docs/api/java/util/concurrent/locks/Lock.html
-- This message is automatically generated by JIRA. - If you think it was sent incorrectly contact one of the administrators: http://jira.qos.ch/secure/Administrators.jspa - For more information on JIRA, see: http://www.atlassian.com/software/jira
[ http://jira.qos.ch/browse/LBCORE-67?page=com.atlassian.jira.plugin.system.is... ] Ceki Gulcu edited comment on LBCORE-67 at 10/28/08 12:55 PM: ------------------------------------------------------------- Joern, Thank you for expressing your concern. Have you tried creating a test case where the current code would "bomb"? I am actually quite interested in a test forcing an error, especially because I think that if existed it would be useful in other circumstances. However, I also think that it would not be so easy to come up with such code. :-) was (Author: noreply.ceki@qos.ch): Joern, Thank you for expressing your concern. Have you tried creating a test case where the current code would "bomb"?
Unsecure usage of locks in AppenderAttachableImpl -------------------------------------------------
Key: LBCORE-67 URL: http://jira.qos.ch/browse/LBCORE-67 Project: logback-core Issue Type: Bug Components: Appender Affects Versions: 0.9.10 Reporter: Joern Huxhorn Assignee: Logback dev list Attachments: LockPatch.patch
The unlock of a lock should, I would even say "must", always be done in a finally block. Otherwise really bad things (deadlock) can happen if an exception is thrown. See http://java.sun.com/j2se/1.5.0/docs/api/java/util/concurrent/locks/Lock.html
-- This message is automatically generated by JIRA. - If you think it was sent incorrectly contact one of the administrators: http://jira.qos.ch/secure/Administrators.jspa - For more information on JIRA, see: http://www.atlassian.com/software/jira
[ http://jira.qos.ch/browse/LBCORE-67?page=com.atlassian.jira.plugin.system.is... ] Ralph Goers edited comment on LBCORE-67 at 10/28/08 3:03 PM: ------------------------------------------------------------- In this case I have to agree with Joern. A test case shouldn't be necessary. All the code being locked must always be in the try block with the unlock in the finally. It is too bad the compiler can' generate an error when it isn't done that way. I should have noticed this and commented on it when the code was committed. This should be fixed asap. was (Author: rgoers@apache.org): In this case I have to agree with Joern. A test case shouldn't be necessary. All the code being locked must always be in the try block with the unlock in the finally. It is too bad the compiler can' generate an error when it isn't done that way.
Unsecure usage of locks in AppenderAttachableImpl -------------------------------------------------
Key: LBCORE-67 URL: http://jira.qos.ch/browse/LBCORE-67 Project: logback-core Issue Type: Bug Components: Appender Affects Versions: 0.9.10 Reporter: Joern Huxhorn Assignee: Logback dev list Attachments: LockPatch.patch
The unlock of a lock should, I would even say "must", always be done in a finally block. Otherwise really bad things (deadlock) can happen if an exception is thrown. See http://java.sun.com/j2se/1.5.0/docs/api/java/util/concurrent/locks/Lock.html
-- This message is automatically generated by JIRA. - If you think it was sent incorrectly contact one of the administrators: http://jira.qos.ch/secure/Administrators.jspa - For more information on JIRA, see: http://www.atlassian.com/software/jira
[ http://jira.qos.ch/browse/LBCORE-67?page=com.atlassian.jira.plugin.system.is... ] Ralph Goers commented on LBCORE-67: ----------------------------------- In this case I have to agree with Joern. A test case shouldn't be necessary. All the code being locked must always be in the try block with the unlock in the finally. It is too bad the compiler can' generate an error when it isn't done that way.
Unsecure usage of locks in AppenderAttachableImpl -------------------------------------------------
Key: LBCORE-67 URL: http://jira.qos.ch/browse/LBCORE-67 Project: logback-core Issue Type: Bug Components: Appender Affects Versions: 0.9.10 Reporter: Joern Huxhorn Assignee: Logback dev list Attachments: LockPatch.patch
The unlock of a lock should, I would even say "must", always be done in a finally block. Otherwise really bad things (deadlock) can happen if an exception is thrown. See http://java.sun.com/j2se/1.5.0/docs/api/java/util/concurrent/locks/Lock.html
-- This message is automatically generated by JIRA. - If you think it was sent incorrectly contact one of the administrators: http://jira.qos.ch/secure/Administrators.jspa - For more information on JIRA, see: http://www.atlassian.com/software/jira
[ http://jira.qos.ch/browse/LBCORE-67?page=com.atlassian.jira.plugin.system.is... ] Ceki Gulcu commented on LBCORE-67: ---------------------------------- The code where the locks are not in try/finally blocks are places where exceptions cannot occur, unless the JVM runs out of memory. Nevertheless, I am not against changing the code. My challenge still stands. Can you force this production unworthy code to bomb?
Unsecure usage of locks in AppenderAttachableImpl -------------------------------------------------
Key: LBCORE-67 URL: http://jira.qos.ch/browse/LBCORE-67 Project: logback-core Issue Type: Bug Components: Appender Affects Versions: 0.9.10 Reporter: Joern Huxhorn Assignee: Logback dev list Attachments: LockPatch.patch
The unlock of a lock should, I would even say "must", always be done in a finally block. Otherwise really bad things (deadlock) can happen if an exception is thrown. See http://java.sun.com/j2se/1.5.0/docs/api/java/util/concurrent/locks/Lock.html
-- This message is automatically generated by JIRA. - If you think it was sent incorrectly contact one of the administrators: http://jira.qos.ch/secure/Administrators.jspa - For more information on JIRA, see: http://www.atlassian.com/software/jira
[ http://jira.qos.ch/browse/LBCORE-67?page=com.atlassian.jira.plugin.system.is... ] Joern Huxhorn updated LBCORE-67: -------------------------------- Attachment: LockPatchWithTest.patch Ok, I implemented to deadlock-tests. While it's not a realistic test the same situation could definitely happen in production in case of OutOfMemoryError as I wrote in the comment of the test. It's generally a very bad idea to save a bit of standard code just because a possible problem isn't imaginable at the moment. I'm not even sure if the code without try {} finally {} will execute faster or if there won't be any difference at all. This is one of the cases where I'll really welcome closures in java because those will eliminate this shortcoming, i.e. the manual handling of locks. In the future, code will look like this: lock(readLock) { // do something } instead of readLock.lock(); try { // do something } finally { readLock.unlock(); } but in the meantime we'll just have to stick to conventions.
Unsecure usage of locks in AppenderAttachableImpl -------------------------------------------------
Key: LBCORE-67 URL: http://jira.qos.ch/browse/LBCORE-67 Project: logback-core Issue Type: Bug Components: Appender Affects Versions: 0.9.10 Reporter: Joern Huxhorn Assignee: Logback dev list Attachments: LockPatch.patch, LockPatchWithTest.patch
The unlock of a lock should, I would even say "must", always be done in a finally block. Otherwise really bad things (deadlock) can happen if an exception is thrown. See http://java.sun.com/j2se/1.5.0/docs/api/java/util/concurrent/locks/Lock.html
-- This message is automatically generated by JIRA. - If you think it was sent incorrectly contact one of the administrators: http://jira.qos.ch/secure/Administrators.jspa - For more information on JIRA, see: http://www.atlassian.com/software/jira
[ http://jira.qos.ch/browse/LBCORE-67?page=com.atlassian.jira.plugin.system.is... ] Joern Huxhorn edited comment on LBCORE-67 at 10/28/08 4:16 PM: --------------------------------------------------------------- Ok, I implemented two deadlock-tests. While it's not a realistic test the same situation could definitely happen in production in case of OutOfMemoryError as I wrote in the comment of the test. It's generally a very bad idea to save a bit of standard code just because a possible problem isn't imaginable at the moment. I'm not even sure if the code without try {} finally {} will execute faster or if there won't be any difference at all. This is one of the cases where I'll really welcome closures in java because those will eliminate this shortcoming, i.e. the manual handling of locks. In the future, code will look like this: lock(readLock) { // do something } instead of readLock.lock(); try { // do something } finally { readLock.unlock(); } but in the meantime we'll just have to stick to conventions. was (Author: jhuxhorn): Ok, I implemented to deadlock-tests. While it's not a realistic test the same situation could definitely happen in production in case of OutOfMemoryError as I wrote in the comment of the test. It's generally a very bad idea to save a bit of standard code just because a possible problem isn't imaginable at the moment. I'm not even sure if the code without try {} finally {} will execute faster or if there won't be any difference at all. This is one of the cases where I'll really welcome closures in java because those will eliminate this shortcoming, i.e. the manual handling of locks. In the future, code will look like this: lock(readLock) { // do something } instead of readLock.lock(); try { // do something } finally { readLock.unlock(); } but in the meantime we'll just have to stick to conventions.
Unsecure usage of locks in AppenderAttachableImpl -------------------------------------------------
Key: LBCORE-67 URL: http://jira.qos.ch/browse/LBCORE-67 Project: logback-core Issue Type: Bug Components: Appender Affects Versions: 0.9.10 Reporter: Joern Huxhorn Assignee: Logback dev list Attachments: LockPatch.patch, LockPatchWithTest.patch
The unlock of a lock should, I would even say "must", always be done in a finally block. Otherwise really bad things (deadlock) can happen if an exception is thrown. See http://java.sun.com/j2se/1.5.0/docs/api/java/util/concurrent/locks/Lock.html
-- This message is automatically generated by JIRA. - If you think it was sent incorrectly contact one of the administrators: http://jira.qos.ch/secure/Administrators.jspa - For more information on JIRA, see: http://www.atlassian.com/software/jira
[ http://jira.qos.ch/browse/LBCORE-67?page=com.atlassian.jira.plugin.system.is... ] Joern Huxhorn updated LBCORE-67: -------------------------------- Attachment: LockPatchWithTest2.patch Updated patch against latest rev, no other changes. I got a conflict while updating so I thought I resubmit to make applying the patch less cumbersome.
Unsecure usage of locks in AppenderAttachableImpl -------------------------------------------------
Key: LBCORE-67 URL: http://jira.qos.ch/browse/LBCORE-67 Project: logback-core Issue Type: Bug Components: Appender Affects Versions: 0.9.10 Reporter: Joern Huxhorn Assignee: Logback dev list Attachments: LockPatch.patch, LockPatchWithTest.patch, LockPatchWithTest2.patch
The unlock of a lock should, I would even say "must", always be done in a finally block. Otherwise really bad things (deadlock) can happen if an exception is thrown. See http://java.sun.com/j2se/1.5.0/docs/api/java/util/concurrent/locks/Lock.html
-- This message is automatically generated by JIRA. - If you think it was sent incorrectly contact one of the administrators: http://jira.qos.ch/secure/Administrators.jspa - For more information on JIRA, see: http://www.atlassian.com/software/jira
[ http://jira.qos.ch/browse/LBCORE-67?page=com.atlassian.jira.plugin.system.is... ] Ceki Gulcu resolved LBCORE-67. ------------------------------ Fix Version/s: 0.9.12 Resolution: Fixed fixed
Unsecure usage of locks in AppenderAttachableImpl -------------------------------------------------
Key: LBCORE-67 URL: http://jira.qos.ch/browse/LBCORE-67 Project: logback-core Issue Type: Bug Components: Appender Affects Versions: 0.9.10 Reporter: Joern Huxhorn Assignee: Logback dev list Fix For: 0.9.12
Attachments: LockPatch.patch, LockPatchWithTest.patch, LockPatchWithTest2.patch
The unlock of a lock should, I would even say "must", always be done in a finally block. Otherwise really bad things (deadlock) can happen if an exception is thrown. See http://java.sun.com/j2se/1.5.0/docs/api/java/util/concurrent/locks/Lock.html
-- This message is automatically generated by JIRA. - If you think it was sent incorrectly contact one of the administrators: http://jira.qos.ch/secure/Administrators.jspa - For more information on JIRA, see: http://www.atlassian.com/software/jira
participants (3)
-
Ceki Gulcu (JIRA) -
Joern Huxhorn (JIRA)
-
Ralph Goers (JIRA)