Re: Deadlock between FileHandler and ConsoleHandler when using customized formatter
On 12/9/2013 4:28 PM, Peter Levart wrote: On 12/09/2013 08:02 AM, Shi Jun Zhang wrote: Peter, I think you are misunderstanding this problem. This deadlock is not related to the formatter synchronization, we can make CustomerFormatter.format not synchronized and not call super.format, the deadlock still happens. I'm not saying that your formatters are explicitly synchronized - all formatters are currently effectively synchronized by LogHandlers. The Formatter is invoked from within LogHandler's publish() method which is synchronized (on LogHandler.this). If formatters were invoked out of this synchronized section, there would be no danger of deadlocks when using Logger.log from within custom formatters. But then other issues would arise as a consequence of non-multithreaded formatters being invoked concurrently... Regards, Peter Hi Peter, We have thought about moving formatter out of the synchronized section of Handler.publish(), it can avoid the deadlock. However, we can reproduce the similar deadlock by extending the Writer in Handler and using logger in the customized Writer. -- Regards, Shi Jun Zhang
Re: Deadlock between FileHandler and ConsoleHandler when using customized formatter
On 12/9/2013 4:40 PM, Peter Levart wrote: On 12/09/2013 09:28 AM, Peter Levart wrote: On 12/09/2013 08:02 AM, Shi Jun Zhang wrote: Peter, I think you are misunderstanding this problem. This deadlock is not related to the formatter synchronization, we can make CustomerFormatter.format not synchronized and not call super.format, the deadlock still happens. I'm not saying that your formatters are explicitly synchronized - all formatters are currently effectively synchronized by LogHandlers. The Formatter is invoked from within LogHandler's publish() method which is synchronized (on LogHandler.this). If formatters were invoked out of this synchronized section, there would be no danger of deadlocks when using Logger.log from within custom formatters. But then other issues would arise as a consequence of non-multithreaded formatters being invoked concurrently... Regards, Peter I think the best solution to your problem (or a work-around if you say so) was suggested by Jason Mehrens few messages ago - decoupling of user code from publishing of log records - by creating an AsyncLogHandler that is just feeding a FIFO queue with log records which are processed by a background thread by pop-ing them out of queue and sticking them in the actual LogHandler.publish(). If the queue is unbouded or large enough so that it never fills up, there's no danger of dead-locks even if you invoke Logger.log from custom formatters in such background publishing thread. Regards, Peter Hi Peter, Would the following situation happen if we use AsyncLogHandler? Some error happens and it causes the jvm crashes or System.exit() is called, however the related log record which contains important message is still in the queue and not printed. If so, I think it's unacceptable. -- Regards, Shi Jun Zhang
Re: Deadlock between FileHandler and ConsoleHandler when using customized formatter
On 12/9/2013 8:04 PM, Peter Levart wrote: On 12/09/2013 10:50 AM, Shi Jun Zhang wrote: On 12/9/2013 4:40 PM, Peter Levart wrote: On 12/09/2013 09:28 AM, Peter Levart wrote: On 12/09/2013 08:02 AM, Shi Jun Zhang wrote: Peter, I think you are misunderstanding this problem. This deadlock is not related to the formatter synchronization, we can make CustomerFormatter.format not synchronized and not call super.format, the deadlock still happens. I'm not saying that your formatters are explicitly synchronized - all formatters are currently effectively synchronized by LogHandlers. The Formatter is invoked from within LogHandler's publish() method which is synchronized (on LogHandler.this). If formatters were invoked out of this synchronized section, there would be no danger of deadlocks when using Logger.log from within custom formatters. But then other issues would arise as a consequence of non-multithreaded formatters being invoked concurrently... Regards, Peter I think the best solution to your problem (or a work-around if you say so) was suggested by Jason Mehrens few messages ago - decoupling of user code from publishing of log records - by creating an AsyncLogHandler that is just feeding a FIFO queue with log records which are processed by a background thread by pop-ing them out of queue and sticking them in the actual LogHandler.publish(). If the queue is unbouded or large enough so that it never fills up, there's no danger of dead-locks even if you invoke Logger.log from custom formatters in such background publishing thread. Regards, Peter Hi Peter, Would the following situation happen if we use AsyncLogHandler? Some error happens and it causes the jvm crashes or System.exit() is called, however the related log record which contains important message is still in the queue and not printed. If so, I think it's unacceptable. You could install a shutdown-hook that would make sure the remaining queue is flushed before completing the VM exit... Regards, Peter But shutdown hook will not be executed if VM crashes or exit abnormally. -- Regards, Shi Jun Zhang
Re: Deadlock between FileHandler and ConsoleHandler when using customized formatter
On 12/9/2013 10:01 PM, Peter Levart wrote: On 12/09/2013 11:12 AM, Daniel Fuchs wrote: On 12/9/13 9:58 AM, Peter Levart wrote: On 12/09/2013 09:51 AM, Shi Jun Zhang wrote: On 12/9/2013 4:28 PM, Peter Levart wrote: On 12/09/2013 08:02 AM, Shi Jun Zhang wrote: Peter, I think you are misunderstanding this problem. This deadlock is not related to the formatter synchronization, we can make CustomerFormatter.format not synchronized and not call super.format, the deadlock still happens. I'm not saying that your formatters are explicitly synchronized - all formatters are currently effectively synchronized by LogHandlers. The Formatter is invoked from within LogHandler's publish() method which is synchronized (on LogHandler.this). If formatters were invoked out of this synchronized section, there would be no danger of deadlocks when using Logger.log from within custom formatters. But then other issues would arise as a consequence of non-multithreaded formatters being invoked concurrently... Regards, Peter Hi Peter, We have thought about moving formatter out of the synchronized section of Handler.publish(), it can avoid the deadlock. However, we can reproduce the similar deadlock by extending the Writer in Handler and using logger in the customized Writer. That's right. And the remedy for that situation would also be what Jason Mehrens suggested - asynchronous publishing. Hi, I agree with Peter Jason - asynchronous publishing seems like a good solution. I believe the LogManager will close all handlers on exiting, so you might want to make sure that your asynchronous handler flushes the queue before quitting - which could still be tricky if flushing the queue produces new log messages for the queue - and also because you will want Handler.close() to wait until the queue is empty. You're right, Daniel. There already is a global shut-down hook installed in LogManager that close()s all active handlers when VM is shutting down. Shi Jun Zhang, here's a quick mock-up of a prototype AsyncHandler that might work for you: http://cr.openjdk.java.net/~plevart/misc/jul.AsyncHandler/AsyncHandler.java Regards, Peter Thanks Peter for your prototype. I would like to see this AsyncHandler to be added into Java 8 or 9. However we will not use this in our stable product as we already have several other low risk workarounds. Anyway - the best advice still is IMHO - don't call Logger.log while publishing a log message. This should save you from a lot of issues, like the one you encountered - but also possible stack overflows etc... Yes, I totally agree this, so I suggest documenting this and let other Java developers not face this problem again. best regards, -- daniel Regards, Peter -- Regards, Shi Jun Zhang
Re: Deadlock between FileHandler and ConsoleHandler when using customized formatter
On 12/6/2013 12:53 AM, Jason Mehrens wrote: Shi Jun Zhang, This problem is like hooking up your sink drain to your sink faucet. Even if it you can get it to work you still would not want to use it. In your code example you could just pre-pend the thread name to the formatter string and return it. Hi Jason, This would work and we already have several workarounds including this one. However, if you really, really, really, want to attempt this type of trick you have to use JDK8 and create a custom Filter to generate log records. If you use a filter, you can ditch the thread local in favor of checking the source class of the log record to determine the proper action. Or if you are using a JDK prior to JDK8 then install the filter on the logger instead of the handler to avoid the locking. I don't see how Filter can avoid the deadlock, Handler.isLoggable() is still called inside Handler.publish(). As long as we call Logger.log inside itself with 2 or more handlers in root logger, the deadlock would happen. Peter, Daniel, An optimistic tail string would break all formatters that write out summary statistics (num of records, min millis, and max mills). Instead of changing the formatter synchronization, I think a more useful solution would be to create an AsyncHandler that would disconnect user code from the act of publishing log records. Then all of handler and formatter locking fall rights into the nice biased locking scenario. Peter, I think you are misunderstanding this problem. This deadlock is not related to the formatter synchronization, we can make CustomerFormatter.format not synchronized and not call super.format, the deadlock still happens. Jason Hi Shi Jun Zhang, I have looked at this, creating a prototype. It re-arranged synchronization in a way so that all Formatter methods are invoked out of synchronized sections. I haven't come forward with this yet, because of two issues: - Formatter implementations would suddenly be called multi-threaded. Currently they are invoked from within Handler-instance synchronized sections. - Formatter would have to be invoked optimistically to obtain head and tail strings, so it could happen that a head, for example, would be requested concurrently multiple times, but only one of returned heads would be written to stream then. The 1st thing seems problematic. I can imagine there exist Formatters that are not thread-safe (for example, using single instance of MessageFormat, which is not multi-threaded) and now just happen to work as a consequence of current StreamHandler implementation detail, but would break if called multi-threaded. One way to remedy this is to add a boolean property to Formatter API, say Formatter.isMultiThreaded(), and arrange so that appropriate instances return appropriate values also considering backwards-compatibility... So all-in-all this is not a simple patch and I doubt it can be made for JDK8. In JDK9, I think, it will be possible to re-visit this issue, so It would be good to file it as a BUG or RFI. Regards, Peter -- Regards, Shi Jun Zhang
Re: Deadlock between FileHandler and ConsoleHandler when using customized formatter
On 11/30/2013 12:05 AM, Daniel Fuchs wrote: On 11/29/13 4:56 PM, Alan Bateman wrote: On 29/11/2013 10:08, Daniel Fuchs wrote: However, removing or just moving the lock around might well introduce new unknown issues - so it will need to be carefully anaIyzed, and I am not sure it can/should be attempted in a minor JDK release. Yes, we have to be very careful as the logging code has a history of biting the hand of those that try to improve it. For various reasons, it seems there is a lot of code that has subtle dependencies on the implementation, on the initialization in particular. In any case, you are to be applauded for tackling the synchronization issues and it would be a good project to re-examine all of this in JDK 9 to see how it would be simplified. On documenting the locking details in an @implNote (which seems to be one of the suggestions here) then we also need to be careful as I think we need some flexibility to change some of this going forward. Yes - that's a two edged sword indeed. We certainly don't want to set that in stone... On the other hand I empathizes with developers who struggle to find out what they can - and can't do - when extending j.u.l APIs... Anyway, if usage of the 'synchronized' keyword never appears in the Javadoc I guess that's for good reasons... Thanks Alan, -- daniel -Alan Hi Daniel, This thread is silent for several days, do you have any finding in Handler.publish? -- Regards, Shi Jun Zhang
Re: Deadlock between FileHandler and ConsoleHandler when using customized formatter
On 11/29/2013 5:25 PM, Daniel Fuchs wrote: On 11/29/13 7:19 AM, Shi Jun Zhang wrote: On 11/29/2013 1:21 AM, Daniel Fuchs wrote: Hi Shi Jun Zhang, I agree with Peter. It is strange that CustomFormatter calls Logger.log. This looks like the source of the deadlock. Hi Daniel, I explained why we call Logger.log in CustomerFormatter in another mail replied to Peter, CustomerFormatter is complicated and we add some debug logging info in it. When we enable the debug level in logging, the deadlock happens. This is the source of the deadlock. Hi Shi Jun Zhang, Since CustomFormatter returns a message string that will be printed in the log, would it be possible for you to add the debug information in that string (for instance at the end of the string - or at the beginning) rather than calling Logger.log from within CustomFormatter, (and hence from within Handler.publish)? best regards, -- daniel Hi Daniel, Yes, this would be another workaround and we already have several workarounds. We'd like to see whether this problem could be solved in JDK level but not in application, or add some Java spec/doc indicating the usage like this could cause possible deadlock. -- Regards, Shi Jun Zhang The Logger API can be customized in many ways - and when you can plugin custom classes and objects you can introduce new opportunity for deadlocks. In my understanding, usually we should not get deadlock between 2 locks in JDK if we don't violate any Java spec or Java API doc. Concerning the Javadoc - I don't know whether that could be satisfactorily improved. In JDK 8, we have a new annotation, @implNote - which can give non normative hints on what the implementation does under the hood. I am not sure whether documenting the locking mechanism that j.u.l uses behind the scenes would be appropriate - or even feasible. I am afraid that trying to describe every lock that is involved in every possible case along every possible code path would be hard to achieve. I think it would be useful to document the locking mechanism with @implNote annotation. However, this problem also happens in OpenJDK 7, and even Oracle JDK 6. I believe the fact that Handler.publish() is usually synchronized is kind of natural: by which I mean that I would naively expect it, given that you wouldn't want one message to overlap with the next. Maybe that could be documented. best regards, -- daniel On 11/28/13 1:13 PM, Peter Levart wrote: On 11/28/2013 08:53 AM, Shi Jun Zhang wrote: The problem is that we use a logger in CustomerFormatter and this causes Logger.log call Logger.log itself. As FileHandler.publish and StreamHandler.publish is synchronized, but the iteration to call publish method for all handlers in Logger.log is not synchronized, the deadlock happens. Hello Shi Jun Zhang, Why do you use Logger.log in the CustomerFormatter? What are you achieving by it? Do you want to re-route and re-format messages destined for one handler to some other Logger and consequently handler? On 11/28/2013 08:53 AM, Shi Jun Zhang wrote: This violates the Java doc for java.util.logging.Logger that says All methods on Logger are multi-thread safe. I don't know for sure, but I think that multi-thread-safe does not imply dead-lock-safe. It would be good if java logging used less locks and be less deadlock-prone though. So we should see if it is possible to remove some locks, not to add more locking... Regards, Peter
Re: Deadlock between FileHandler and ConsoleHandler when using customized formatter
On 11/28/2013 8:13 PM, Peter Levart wrote: On 11/28/2013 08:53 AM, Shi Jun Zhang wrote: The problem is that we use a logger in CustomerFormatter and this causes Logger.log call Logger.log itself. As FileHandler.publish and StreamHandler.publish is synchronized, but the iteration to call publish method for all handlers in Logger.log is not synchronized, the deadlock happens. Hello Shi Jun Zhang, Why do you use Logger.log in the CustomerFormatter? What are you achieving by it? Do you want to re-route and re-format messages destined for one handler to some other Logger and consequently handler? Hi Peter, This happens in a real complicated application and I simply the test case. There is some complicated logic in the CustomerFormatter and we add some debug log messages in CustomerFormatter.format() method. As CustomerFormatter.format() method is called in Logger.log, there would be an infinite recursion if we do nothing, then we have to add some check to break the recursion. The things we are doing here are 1) using CustomerFormatter as logger formatter and logging in an application. 2) logging some debug information in CustomerFormatter. On 11/28/2013 08:53 AM, Shi Jun Zhang wrote: This violates the Java doc for java.util.logging.Logger that says All methods on Logger are multi-thread safe. I don't know for sure, but I think that multi-thread-safe does not imply dead-lock-safe. It would be good if java logging used less locks and be less deadlock-prone though. So we should see if it is possible to remove some locks, not to add more locking... Regards, Peter I agree that we need less locks in java logging, java logging is getting more complicated and getting more deadlock recently. -- Regards, Shi Jun Zhang
Re: Deadlock between FileHandler and ConsoleHandler when using customized formatter
On 11/29/2013 1:21 AM, Daniel Fuchs wrote: Hi Shi Jun Zhang, I agree with Peter. It is strange that CustomFormatter calls Logger.log. This looks like the source of the deadlock. Hi Daniel, I explained why we call Logger.log in CustomerFormatter in another mail replied to Peter, CustomerFormatter is complicated and we add some debug logging info in it. When we enable the debug level in logging, the deadlock happens. This is the source of the deadlock. The Logger API can be customized in many ways - and when you can plugin custom classes and objects you can introduce new opportunity for deadlocks. In my understanding, usually we should not get deadlock between 2 locks in JDK if we don't violate any Java spec or Java API doc. Concerning the Javadoc - I don't know whether that could be satisfactorily improved. In JDK 8, we have a new annotation, @implNote - which can give non normative hints on what the implementation does under the hood. I am not sure whether documenting the locking mechanism that j.u.l uses behind the scenes would be appropriate - or even feasible. I am afraid that trying to describe every lock that is involved in every possible case along every possible code path would be hard to achieve. I think it would be useful to document the locking mechanism with @implNote annotation. However, this problem also happens in OpenJDK 7, and even Oracle JDK 6. I believe the fact that Handler.publish() is usually synchronized is kind of natural: by which I mean that I would naively expect it, given that you wouldn't want one message to overlap with the next. Maybe that could be documented. best regards, -- daniel On 11/28/13 1:13 PM, Peter Levart wrote: On 11/28/2013 08:53 AM, Shi Jun Zhang wrote: The problem is that we use a logger in CustomerFormatter and this causes Logger.log call Logger.log itself. As FileHandler.publish and StreamHandler.publish is synchronized, but the iteration to call publish method for all handlers in Logger.log is not synchronized, the deadlock happens. Hello Shi Jun Zhang, Why do you use Logger.log in the CustomerFormatter? What are you achieving by it? Do you want to re-route and re-format messages destined for one handler to some other Logger and consequently handler? On 11/28/2013 08:53 AM, Shi Jun Zhang wrote: This violates the Java doc for java.util.logging.Logger that says All methods on Logger are multi-thread safe. I don't know for sure, but I think that multi-thread-safe does not imply dead-lock-safe. It would be good if java logging used less locks and be less deadlock-prone though. So we should see if it is possible to remove some locks, not to add more locking... Regards, Peter -- Regards, Shi Jun Zhang
Deadlock between FileHandler and ConsoleHandler when using customized formatter
Hi, We get a deadlock between java.util.logging.FileHandler and java.util.logging.ConsoleHandler when we are using a customized formatter in these handlers. Here is the simple test case. http://cr.openjdk.java.net/~zhangshj/deadlockInHandlers/deadlockInHandlers.zip Run java -Djava.util.logging.config.file=logging.properties TestThread, it will hang immediately. And this is the stack trace in dump file Thread-1: at java.util.logging.StreamHandler.publish(StreamHandler.java:205) - waiting to lock 0xec52e0c0 (a java.util.logging.ConsoleHandler) at java.util.logging.ConsoleHandler.publish(ConsoleHandler.java:118) at java.util.logging.Logger.log(Logger.java:617) at java.util.logging.Logger.doLog(Logger.java:638) at java.util.logging.Logger.log(Logger.java:661) at java.util.logging.Logger.info(Logger.java:1289) at CustomerFormatter.format(CustomerFormatter.java:16) at java.util.logging.StreamHandler.publish(StreamHandler.java:210) - locked 0xec51ab90 (a java.util.logging.FileHandler) at java.util.logging.FileHandler.publish(FileHandler.java:614) - locked 0xec51ab90 (a java.util.logging.FileHandler) at java.util.logging.Logger.log(Logger.java:617) at java.util.logging.Logger.doLog(Logger.java:638) at java.util.logging.Logger.log(Logger.java:661) at java.util.logging.Logger.info(Logger.java:1289) at TestThread$1.run(TestThread.java:14) at java.lang.Thread.run(Thread.java:724) Thread-2: at java.util.logging.FileHandler.publish(FileHandler.java:611) - waiting to lock 0xec51ab90 (a java.util.logging.FileHandler) at java.util.logging.Logger.log(Logger.java:617) at java.util.logging.Logger.doLog(Logger.java:638) at java.util.logging.Logger.log(Logger.java:661) at java.util.logging.Logger.info(Logger.java:1289) at CustomerFormatter.format(CustomerFormatter.java:16) at java.util.logging.StreamHandler.publish(StreamHandler.java:210) - locked 0xec52e0c0 (a java.util.logging.ConsoleHandler) at java.util.logging.ConsoleHandler.publish(ConsoleHandler.java:118) at java.util.logging.Logger.log(Logger.java:617) at java.util.logging.Logger.doLog(Logger.java:638) at java.util.logging.Logger.log(Logger.java:661) at java.util.logging.Logger.info(Logger.java:1289) at TestThread$1.run(TestThread.java:14) at java.lang.Thread.run(Thread.java:724) The problem is that we use a logger in CustomerFormatter and this causes Logger.log call Logger.log itself. As FileHandler.publish and StreamHandler.publish is synchronized, but the iteration to call publish method for all handlers in Logger.log is not synchronized, the deadlock happens. This violates the Java doc for java.util.logging.Logger that says All methods on Logger are multi-thread safe. Currently we have 2 workarounds. First is to add 2 lines in the logging.properties to force CustomerFormatter use new instance of FileHandler and ConsoleHandler like this, CustomerFormatter.handlers = java.util.logging.FileHandler, java.util.logging.ConsoleHandler CustomerFormatter.useParentHandlers = false Second is to synchronize the logger.info call in TestThread test case. I'm not sure whether we should fix this problem in JDK. The fix I can image is to synchronize each call to handler.publish() in Logger.log, but I think this would cause performance degradation. Another solution is to add some documents in Java doc to state that Logger is not so much multi-thread safe. Any suggestion or better fix? -- Regards, Shi Jun Zhang
Re: RFR: 8019381: HashMap.isEmpty is non-final, potential issues for get/remove
On 7/5/2013 10:58 AM, Jonathan Lu wrote: On 07/03/2013 11:04 AM, Shi Jun Zhang wrote: On 7/1/2013 11:49 PM, Chris Hegarty wrote: On 1 Jul 2013, at 17:22, Remi Forax fo...@univ-mlv.fr wrote: On 07/01/2013 09:43 AM, Shi Jun Zhang wrote: On 6/29/2013 12:05 AM, Shi Jun Zhang wrote: On 6/28/2013 9:02 PM, Alan Bateman wrote: On 27/06/2013 22:13, Remi Forax wrote: On 06/27/2013 10:02 AM, Shi Jun Zhang wrote: Hi, There are some isEmpty() check added into get/remove methods since 8011200 to return directly if HashMap is empty. However isEmpty is a non-final public method which can be overridden by subclass. If the subclass defines isEmpty differently from HashMap, it would cause problem while getting or removing elements. yes, it's a bug. Could you report it ? Rémi I've created a bug to track this: 8019381: HashMap.isEmpty is non-final, potential issues for get/remove -Alan Thanks, Alan. I'm quite busy today and do not have time to report it until now. Thanks for your help. I will provide a webrev next Monday for review. Hi, Here is the webrev http://cr.openjdk.java.net/~zhangshj/8019381/webrev.00/ This looks Ok for me. +1 -Chris Rémi Thanks all for the review. Jonathan, Could you help to push the changeset? Hello Chance, Patch pushed @ http://hg.openjdk.java.net/jdk8/tl/jdk/rev/ed111451b77a, pls verify. Cheers! - Jonathan Thanks, Jonathan. -- Regards, Shi Jun Zhang
Re: RFR: 8019381: HashMap.isEmpty is non-final, potential issues for get/remove
On 7/1/2013 11:49 PM, Chris Hegarty wrote: On 1 Jul 2013, at 17:22, Remi Forax fo...@univ-mlv.fr wrote: On 07/01/2013 09:43 AM, Shi Jun Zhang wrote: On 6/29/2013 12:05 AM, Shi Jun Zhang wrote: On 6/28/2013 9:02 PM, Alan Bateman wrote: On 27/06/2013 22:13, Remi Forax wrote: On 06/27/2013 10:02 AM, Shi Jun Zhang wrote: Hi, There are some isEmpty() check added into get/remove methods since 8011200 to return directly if HashMap is empty. However isEmpty is a non-final public method which can be overridden by subclass. If the subclass defines isEmpty differently from HashMap, it would cause problem while getting or removing elements. yes, it's a bug. Could you report it ? Rémi I've created a bug to track this: 8019381: HashMap.isEmpty is non-final, potential issues for get/remove -Alan Thanks, Alan. I'm quite busy today and do not have time to report it until now. Thanks for your help. I will provide a webrev next Monday for review. Hi, Here is the webrev http://cr.openjdk.java.net/~zhangshj/8019381/webrev.00/ This looks Ok for me. +1 -Chris Rémi Thanks all for the review. Jonathan, Could you help to push the changeset? -- Regards, Shi Jun Zhang
RFR: 8019381: HashMap.isEmpty is non-final, potential issues for get/remove
On 6/29/2013 12:05 AM, Shi Jun Zhang wrote: On 6/28/2013 9:02 PM, Alan Bateman wrote: On 27/06/2013 22:13, Remi Forax wrote: On 06/27/2013 10:02 AM, Shi Jun Zhang wrote: Hi, There are some isEmpty() check added into get/remove methods since 8011200 to return directly if HashMap is empty. However isEmpty is a non-final public method which can be overridden by subclass. If the subclass defines isEmpty differently from HashMap, it would cause problem while getting or removing elements. yes, it's a bug. Could you report it ? Rémi I've created a bug to track this: 8019381: HashMap.isEmpty is non-final, potential issues for get/remove -Alan Thanks, Alan. I'm quite busy today and do not have time to report it until now. Thanks for your help. I will provide a webrev next Monday for review. Hi, Here is the webrev http://cr.openjdk.java.net/~zhangshj/8019381/webrev.00/ -- Regards, Shi Jun Zhang
Re: Question on HashMap change in 8011200
On 6/28/2013 9:02 PM, Alan Bateman wrote: On 27/06/2013 22:13, Remi Forax wrote: On 06/27/2013 10:02 AM, Shi Jun Zhang wrote: Hi, There are some isEmpty() check added into get/remove methods since 8011200 to return directly if HashMap is empty. However isEmpty is a non-final public method which can be overridden by subclass. If the subclass defines isEmpty differently from HashMap, it would cause problem while getting or removing elements. yes, it's a bug. Could you report it ? Rémi I've created a bug to track this: 8019381: HashMap.isEmpty is non-final, potential issues for get/remove -Alan Thanks, Alan. I'm quite busy today and do not have time to report it until now. Thanks for your help. I will provide a webrev next Monday for review. -- Regards, Shi Jun Zhang
Question on HashMap change in 8011200
Hi,
There are some isEmpty() check added into get/remove methods since
8011200 to return directly if HashMap is empty. However isEmpty is a
non-final public method which can be overridden by subclass. If the
subclass defines isEmpty differently from HashMap, it would cause
problem while getting or removing elements.
For example, here is a simple test case, the subclass of HashMap always
has at least one key value pair so isEmpty will never be false.
///
import java.util.HashMap;
public class HashMapTest {
public static class NotEmptyHashMapK,V extends HashMapK,V {
private K alwaysExistingKey;
private V alwaysExistingValue;
@Override
public V get(Object key) {
if (key == alwaysExistingKey) {
return alwaysExistingValue;
}
return super.get(key);
}
@Override
public int size() {
return super.size() + 1;
}
@Override
public boolean isEmpty() {
return size() == 0;
}
}
public static void main(String[] args) {
NotEmptyHashMapString, String map = new NotEmptyHashMap();
map.get(key);
}
}
//
The test can end successfully before 8011200 but it will throw
ArrayIndexOutOfBoundsException after 8011200. The reason is isEmpty()
check in HashMap.getEntry() method gets passed but the actual table
length is 0. I think the real intention of isEmpty() check is to check
whether size in HashMap is 0 but not whether the subclass is empty, so I
suggest to use size == 0 instead of isEmpty() in get/remove methods.
Do you think this is a bug or a wrong usage of extending HashMap? I find
there is a similar usage in javax.swing.MultiUIDefaults which extends
Hashtable.
--
Regards,
Shi Jun Zhang
Re: 7099119: Remove unused dlinfo local variable in launcher code
On 11/30/2012 5:27 PM, Jonathan Lu wrote: On 11/30/2012 04:58 PM, Alan Bateman wrote: On 30/11/2012 08:49, Jonathan Lu wrote: Hi Chance, I'm getting an error of, remote: Bugid 7099119 already used in this repository, in revision 4607 Could you please create another sun bug for this issue? Right, you can't use a bug/issue number in more than one change-set in the same repository. I've submitted another one that you can use: JDK-8004211: Remove unused dlinfo local variable in launcher code -Alan Thanks, Alan. The patch has been pushed to http://hg.openjdk.java.net/jdk8/tl/jdk/rev/e988de7465d4 Best regards! - Jonathan Thanks, Alan and Jonathan. -- Regards, Shi Jun Zhang
Re: 7099119: Remove unused dlinfo local variable in launcher code
On 11/29/2012 6:40 PM, Alan Bateman wrote: On 29/11/2012 07:17, Shi Jun wrote: Hi all, Previously 7099119 is fixed in the following changeset: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/dd55467dd1f2, but this change is lost during the Mac port merging changes 7113349 and 7146424. Hereby I raise this thread again. Webrev: http://cr.openjdk.java.net/~zhangshj/7099119/webrev.00/ Previous discuss thread: http://mail.openjdk.java.net/pipermail/core-libs-dev/2011-October/007872.html I don't know this slipped it, perhaps it was due to the refactoring. In any case your change looks fine. -Alan Thanks, Alan. Hi Jonathan, Could you help to push the change? -- Regards, Shi Jun Zhang
Re: RFR: 7155300 Include pthread.h on all POSIX platforms except Solaris to improve portability
On 3/22/2012 12:36 PM, Charles Lee wrote: On 03/21/2012 10:47 AM, Shi Jun Zhang wrote: On 3/12/2012 11:28 AM, Shi Jun Zhang wrote: On 3/9/2012 6:05 PM, David Holmes wrote: On 9/03/2012 7:04 PM, Alan Bateman wrote: On 09/03/2012 08:01, Shi Jun Zhang wrote: The situation in NativeThread.c is more complicated than other 2 files. I'm not familiar with BSD or Mac. It seems that we don't need to signal threads on BSD or Mac. And INTERRUPT_SIGNAL on AIX will definitely be different from the one on Linux. I think we'd better separate the changes in NativeThread.c from this patch and try to solve it later. Right, if signals are required there is likely to be differences across platforms. It is also likely that this code will need to be changed for Mac too as there are a couple of preemptive close issues to sort out (for file operations, sockets are okay). The change to socket_md.c looks okay to me but you will need to re-base your patch due to the Mac port in jdk8/tl. I'm a new comer and i got known from Charles about the difference between jdk8 and jdk8/tl. The latest webrev is based on jdk8/tl. http://cr.openjdk.java.net/~zhangshj/pthread/webrev.01/ The changes in this webrev look okay to me. In java_md.c 1445 /* See above. Continue in current thread if thr_create() failed */ The see above is now a see below. I think the launcher changes are okay because BSD/OSX won't use this file. I think the socket changes are okay as long as BSD builds and OSX builds define _ALLBSD_SOURCE. I still don't fully understand if a BSD build and an OSX build are distinct. David -Alan. The comment see above has been changed to see below. http://cr.openjdk.java.net/~zhangshj/pthread/webrev.02/ Hi Alan/David, There is no response on this thread for long time. I created a sun bug 7155300, could you help to review it? The webrev link is http://cr.openjdk.java.net/~zhangshj/pthread/webrev.02/ Hi Chance, Here is the changeset. Please verify it. Changeset: 1d418ec212ea Author:zhangshj Date: 2012-03-22 12:30 +0800 URL:http://hg.openjdk.java.net/jdk8/tl/jdk/rev/1d418ec212ea 7155300: Include pthread.h on all POSIX platforms except Solaris to improve portability Reviewed-by: alanb, dholmes ! src/solaris/bin/java_md.c ! src/solaris/transport/socket/socket_md.c Thank you all for reviewing. Looks good. Thanks all for reviewing and committing. -- Regards, Shi Jun Zhang
Re: RFR: 7155300 Include pthread.h on all POSIX platforms except Solaris to improve portability
On 3/12/2012 11:28 AM, Shi Jun Zhang wrote: On 3/9/2012 6:05 PM, David Holmes wrote: On 9/03/2012 7:04 PM, Alan Bateman wrote: On 09/03/2012 08:01, Shi Jun Zhang wrote: The situation in NativeThread.c is more complicated than other 2 files. I'm not familiar with BSD or Mac. It seems that we don't need to signal threads on BSD or Mac. And INTERRUPT_SIGNAL on AIX will definitely be different from the one on Linux. I think we'd better separate the changes in NativeThread.c from this patch and try to solve it later. Right, if signals are required there is likely to be differences across platforms. It is also likely that this code will need to be changed for Mac too as there are a couple of preemptive close issues to sort out (for file operations, sockets are okay). The change to socket_md.c looks okay to me but you will need to re-base your patch due to the Mac port in jdk8/tl. I'm a new comer and i got known from Charles about the difference between jdk8 and jdk8/tl. The latest webrev is based on jdk8/tl. http://cr.openjdk.java.net/~zhangshj/pthread/webrev.01/ The changes in this webrev look okay to me. In java_md.c 1445 /* See above. Continue in current thread if thr_create() failed */ The see above is now a see below. I think the launcher changes are okay because BSD/OSX won't use this file. I think the socket changes are okay as long as BSD builds and OSX builds define _ALLBSD_SOURCE. I still don't fully understand if a BSD build and an OSX build are distinct. David -Alan. The comment see above has been changed to see below. http://cr.openjdk.java.net/~zhangshj/pthread/webrev.02/ Hi Alan/David, There is no response on this thread for long time. I created a sun bug 7155300, could you help to review it? The webrev link is http://cr.openjdk.java.net/~zhangshj/pthread/webrev.02/ -- Regards, Shi Jun Zhang
Re: Suggestion about including pthread.h
On 3/9/2012 6:05 PM, David Holmes wrote: On 9/03/2012 7:04 PM, Alan Bateman wrote: On 09/03/2012 08:01, Shi Jun Zhang wrote: The situation in NativeThread.c is more complicated than other 2 files. I'm not familiar with BSD or Mac. It seems that we don't need to signal threads on BSD or Mac. And INTERRUPT_SIGNAL on AIX will definitely be different from the one on Linux. I think we'd better separate the changes in NativeThread.c from this patch and try to solve it later. Right, if signals are required there is likely to be differences across platforms. It is also likely that this code will need to be changed for Mac too as there are a couple of preemptive close issues to sort out (for file operations, sockets are okay). The change to socket_md.c looks okay to me but you will need to re-base your patch due to the Mac port in jdk8/tl. I'm a new comer and i got known from Charles about the difference between jdk8 and jdk8/tl. The latest webrev is based on jdk8/tl. http://cr.openjdk.java.net/~zhangshj/pthread/webrev.01/ The changes in this webrev look okay to me. In java_md.c 1445 /* See above. Continue in current thread if thr_create() failed */ The see above is now a see below. I think the launcher changes are okay because BSD/OSX won't use this file. I think the socket changes are okay as long as BSD builds and OSX builds define _ALLBSD_SOURCE. I still don't fully understand if a BSD build and an OSX build are distinct. David -Alan. The comment see above has been changed to see below. http://cr.openjdk.java.net/~zhangshj/pthread/webrev.02/ -- Regards, Shi Jun Zhang
Re: Suggestion about including pthread.h
On 3/8/2012 7:25 PM, Alan Bateman wrote: On 08/03/2012 08:09, Shi Jun Zhang wrote: There is still no reply from build infra project and even if it is in build infra, it will take a long time to merge back to trunk. But this including pthread problem really affects AIX platform. I'm thinking we can use #ifndef __solaris__ form because all other POSIX-conformant platforms (BSD, Mac, AIX, ...) except Solaris need to include pthread.h. Here is the webrev: http://cr.openjdk.java.net/~zhangshj/pthread/webrev.00/ Kumar will likely have an opinion on java_md.c/ContinueInNewThread0. Personally I would go for #ifdef __solaris ... #end ... #endif rather than #ifndef __solaris__. I've modified it to #ifdef __solaris__ ... #else ... #endif. I don't think the changes to NativeThread.c are quite right because it assumes that we need to signal threads on platforms other than Solaris and it also assumes the signal is __SIGRTMAX - 2. I don't know what you use in your AIX port but once the preemptive close issues are sorted out on Mac then I assume it will be SIGIO. So for this one I would suggest the ifdef in the init, current and signal methods be ifdef INTERRUPT_SIGNAL. The situation in NativeThread.c is more complicated than other 2 files. I'm not familiar with BSD or Mac. It seems that we don't need to signal threads on BSD or Mac. And INTERRUPT_SIGNAL on AIX will definitely be different from the one on Linux. I think we'd better separate the changes in NativeThread.c from this patch and try to solve it later. The change to socket_md.c looks okay to me but you will need to re-base your patch due to the Mac port in jdk8/tl. I'm a new comer and i got known from Charles about the difference between jdk8 and jdk8/tl. The latest webrev is based on jdk8/tl. http://cr.openjdk.java.net/~zhangshj/pthread/webrev.01/ -- Regards, Shi Jun Zhang
Re: Suggestion about including pthread.h
On 3/2/2012 5:05 PM, Alan Bateman wrote: On 02/03/2012 07:53, David Holmes wrote: Yes we need to move to a more capability based inclusion conditional compilation mechanism. I'm not sure if the build-infra project is tackling this particular case. Yes, I think moving to a more capability based compilation is where build-infa wants to go, although clearly it's going to take a long time. As I see, that project will put the infrastructure in place and then it's up to each area to gradually eliminate the ifdef platform usages. I don't think they will all go away but with effort then things should be more portable than what we have now. -Alan. There is still no reply from build infra project and even if it is in build infra, it will take a long time to merge back to trunk. But this including pthread problem really affects AIX platform. I'm thinking we can use #ifndef __solaris__ form because all other POSIX-conformant platforms (BSD, Mac, AIX, ...) except Solaris need to include pthread.h. Here is the webrev: http://cr.openjdk.java.net/~zhangshj/pthread/webrev.00/ -- Regards, Shi Jun Zhang
Re: Suggestion about including pthread.h
On 3/2/2012 3:53 PM, David Holmes wrote: On 2/03/2012 5:05 PM, Shi Jun Zhang wrote: Currently jdk/src/solaris/bin/java_md.c includes pthread.h with #ifdef __linux__, but BSD, MAC OS, AIX all needs to include pthread.h. To avoid the situation that the ifdef clause becomes longer and longer like #if defined(__linux__) || defined(_ALLBSD_SOURCE) || defined(AIX) || defined(OTHER_PLATFORMS), i suggest to use USE_PTHREADS already defined in jdk/make/common/Defs-linux.gmk and add a compiler flag if USE_PTHREADS is true. It will look like this: ifeq ($(USE_PTHREADS), true) CPPFLAGS_COMMON += -DUSE_PTHREADS endif And then all the places need to include pthread.h only needs to use #ifdef USE_PTHREADS. The files include pthread.h are jdk/src/solaris/bin/java_md.c jdk/src/solaris/native/sun/nio/ch/NativeThread.c jdk/src/solaris/transport/socket/socket_md.c Any comments? Yes we need to move to a more capability based inclusion conditional compilation mechanism. I'm not sure if the build-infra project is tackling this particular case. David So add build-infra in the loop. -- Regards, Shi Jun Zhang
Suggestion about including pthread.h
Hi, Currently jdk/src/solaris/bin/java_md.c includes pthread.h with #ifdef __linux__, but BSD, MAC OS, AIX all needs to include pthread.h. To avoid the situation that the ifdef clause becomes longer and longer like #if defined(__linux__) || defined(_ALLBSD_SOURCE) || defined(AIX) || defined(OTHER_PLATFORMS), i suggest to use USE_PTHREADS already defined in jdk/make/common/Defs-linux.gmk and add a compiler flag if USE_PTHREADS is true. It will look like this: ifeq ($(USE_PTHREADS), true) CPPFLAGS_COMMON += -DUSE_PTHREADS endif And then all the places need to include pthread.h only needs to use #ifdef USE_PTHREADS. The files include pthread.h are jdk/src/solaris/bin/java_md.c jdk/src/solaris/native/sun/nio/ch/NativeThread.c jdk/src/solaris/transport/socket/socket_md.c Any comments? -- Regards, Shi Jun Zhang
