This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.
Originally reported as issue 186366, that was already hot-fixed for NB 6.9, but we need better fix for trunk, see Jesse's comment : ------------------ http://netbeans.org/bugzilla/show_bug.cgi?id=186366#c32 Catching Throwable (i.e. AssertionError, but also ThreadDeath, etc. etc.) is not an acceptable long-term fix; it is a hotfix until the cause of the assertion failure can be properly diagnosed. Either the calling code should not "switch to silent mode while not running" (and the assertion should be changed to an IllegalStateException); or, if this is not really an error, the throw site should be changed to just print a warning or do nothing. ------------------
It is an assert. I can change the code to catch AssertError only. The alternative is to play with additional variables to keep information about the state of the handle. I don't see a reason for that, as the progress API seems to be well aware of its state internally.
Not sure why this was closed. Catching Throwable is never acceptable, certainly not catching an assertion - if the assertion is known to fail then something needs to be fixed.
Created attachment 100133 [details] Calling suspend on finished task shall be no-op I believe the ProgressAPI is needlessly strict in the way it enforces call order. Catching the throwable was an easy way to fix that, but Jesse is right, it is better to fix the API itself. Here is my first attempt. Relaxing order in other calls may follow.
Seems OK to me.
OK, I will apply this change on methods toSilent(), toIndeterminate() and toDeterminate() if you agree, but my question is - should I make similar steps in start() and finish() methods? There is no assert - they throws IllegalStateException, but under similar circumstances. e.g. http://netbeans.org/bugzilla/show_bug.cgi?id=187774 public synchronized void finish() { if (state == STATE_INITIALIZED) { throw new IllegalStateException("Cannot finish a task that was never started"); } }
(In reply to comment #5) > I will apply this change on methods toSilent(), toIndeterminate() and > toDeterminate() if you agree I think it would be helpful. As to whether the code should issue warnings when these anomalous state transitions are attempted, I am unsure - on the one hand it would be helpful for debugging callers which really did not intend to call operations in this order; on the other hand it may be annoyance for callers which generally issue operations in the correct order but which may on occasion do things in an odd order due to a race condition and where it would be cumbersome to keep track of the handle's state and provide no real benefit. May be wise to review known places where these errors are thrown today and check to see if the mistakes are indicative of any deeper problem. For example, I got some state-related errors from the Progress API in Maven index download code which led me to understand that under certain circumstances a download would be restarted, and that an event named something like "transferStarted" would actually be issued twice. I think I managed to fix those problems to not just satisfy the Progress API's restrictions, but to actually display the progress correctly (needed to reset the progress bar after the first bogus download was canceled and the real download began). If there were not even a warning in the Progress API it might have been harder to notice what was going on. > should I make similar steps in start() and finish() methods? > > public synchronized void finish() { > if (state == STATE_INITIALIZED) { > throw new IllegalStateException("Cannot finish a task that was never started"); I think so. Be sure to check the Javadoc for ProgressHandle (and AggregateProgressHandle) to ensure that they are clear about what operations may be performed when; currently it is left to the caller's imagination.
Created attachment 110033 [details] Asserts replaced with warning and return
Sorry, bad patch uploaded, thank you TEAM plugin :(
Created attachment 110034 [details] Asserts replaced with warning and return This is the right one. I assume that calling these methods in wrong state is not an error so I replaced asserts and ISE with warnings and returns - I believe we don't want them to continue. I'm not sure if I should add something to Javadoc of ProgressHandle and AggregateProgressHandle.
Looks OK. [JG01] A minor issue is that when a warning of this kind is printed to the log, it may not be obvious what calling code is responsible and should be investigated. One fix is to log a dummy Exception at FINE alongside the single-line WARNING; then you need to enable logging on InternalHandle (and reproduce the bug) to diagnose. A possibly nicer fix is to adapt findCaller from org.netbeans.core.startup.InstalledFileLocatorImpl (probably this should be factored into a utility method in org.openide.util.Exceptions?), so you use e.g. LOG.log(Level.WARNING, "Cannot switch to silent mode when not running at {0}", findCaller()); which has the pleasant effect that the last segment of the message will be hyperlinked when it appears in the IDE's Output Window during 'ant tryme'.
Created attachment 110060 [details] Warnings with findCaller() method Here is new patch, I hope it is what you had in mind. Do you think that one line is enough?
Just a small correction of condition in findCaller(): if (!line.getClassName().matches(".*(InternalHandle|ProgressHandle).*|java[.].+")) { // NOI18N return line.toString(); }
Looks good to me. BTW when a Logger method is passed only one arg, you do not need to write it in an Object[] (unless it is assignable to Throwable in which case this can be used to disambiguate an overload). I wish they had used varargs on these from the beginning but for some reason they did not.
Ok, what do you think about documentation of ProgressHandle/AggregateProgressHandle? I think there should be mentioned when user can call these methods e.g. switchToIndeterminate() should be called when handle is running. But there is a problem - users of this API can´t find out in what state the handle is, see http://netbeans.org/bugzilla/show_bug.cgi?id=124697
(In reply to comment #14) > I think there should be mentioned when > user can call these methods e.g. switchToIndeterminate() should be called when > handle is running. Yes, it should.
(In reply to comment #3) > I believe the ProgressAPI is needlessly strict in the way it enforces call > order. To describe what I meant I wrote an essay: http://wiki.apidesign.org/wiki/Stateful To sumarize: the current order restrictions are too strict. If it make sense to make the API more forgiving (like allowing suspend after finish), we should do it. In case some calling restrictions have to remain, I suggest to create a picture with call state machine and make it part of javadoc. We are not starting from scratch, so converting to stateless API (as advocated in the essay) is not suitable. We would need the keep the old API for compatibility reasons anyway.
So do you think we should ignore wrong order of calling methods (like in patch i attached) or should we try to fix somehow wrong-state situation? e.g. if client calls: ProgressStateful progress = ProgressStateful.create("..."); progress.progress(10); progress.finish(); should we try to call start() internally or just ignore these calls? In this example there could be a problem with total workunits init...
*** Bug 134347 has been marked as a duplicate of this bug. ***
(In reply to comment #17) > progress.progress(10); > progress.finish(); > In this example there could be a problem with total workunits init... I see. This would be strange situation. Hard to recover from such mistake.
(In reply to comment #19) > I see. This would be strange situation. Hard to recover from such mistake. Most of relevant methods have similar problem so it seems to me that ignoring wrong-order calls (and logging warnings) is probably the best way to solve this. In this case call state machine in javadoc is needed. Can I use that javadoc enhancement for state machines that you mentioned in your essey? Or what is a traditional way to do that?
(In reply to comment #20) > Can I use that javadoc enhancement for state machines That was using real annotations, not Javadoc, and I do not think we want to go there. > what is a traditional way to do that? Just document clearly in the public methods the order in which they may be called. It does not seem very complicated, it is just not documented at all now. As previously mentioned, I think the problems are typically not due to misunderstanding of the semantics, but to unusual call ordering caused by some bug in client code, such as mishandled threading. So a simple warning in the log would suffice to alert callers of possible problems.
Created attachment 110295 [details] Final modification in InternaHandle
Created attachment 110296 [details] ProgressHandle javadoc modifications
(In reply to comment #21) > Just document clearly in the public methods the order in which they may be > called. It does not seem very complicated, it is just not documented at all > now. Done. Please review final changes in InternalHandle.java and ProgressHandle.java (javadoc).
Javadoc changes look fine. I am guessing start() could also be documented to be forbidden after it has already been started. Logging changes look mostly OK. Looks like findCaller will correctly ignore stack trace lines inside AggregateProgressHandle but might incorrectly return ProgressContributor. Trailing .* is probably unnecessary (unless there are inner classes - /([$].+)?/) and might produce false positives for a class in another module named e.g. ProgressHandleHelper. May be easier to just ignore anything in packages known to be part of this module or the accompanying UI module: (java|org[.]netbeans[.](api[.]progress|modules[.]progress|progress[.]module))[.].+ (The org.netbeans.progress.module package could probably be renamed to org.netbeans.modules.progress to simplify things a bit, and similarly for org.netbeans.progress.module.resources.) By the way do not use the IDE's "Export Diff" feature to produce patches for review. Ensure your ~/.hgrc specifies diff.git=1 and use 'hg diff'.
Created attachment 110334 [details] ProgressHandle and InternalHandle changes
What happened to this? Seems to have been forgotten.
(In reply to comment #27) > What happened to this? Seems to have been forgotten. Patch is prepared and ready to be committed. So if you haven't any objection, I will push it now.
fix: http://hg.netbeans.org/core-main/rev/b1bd6480d4da
Integrated into 'main-golden' Changeset: http://hg.netbeans.org/main-golden/rev/b1bd6480d4da User: Jan Peska <jpeska@netbeans.org> Log: Issue #186636 - Allow handle.suspend after handle.finish Replace exceptions and asserts with warnings in log in case of illegal state in Progress API