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.

Bug 180458 - Prevent "parallel misuse" of RequestProcessor.getDefault()
Summary: Prevent "parallel misuse" of RequestProcessor.getDefault()
Alias: None
Product: platform
Classification: Unclassified
Component: -- Other -- (show other bugs)
Version: 6.x
Hardware: Other Linux
: P1 normal (vote)
Assignee: Jaroslav Tulach
Keywords: API_REVIEW
Depends on: 40788
  Show dependency tree
Reported: 2010-02-09 01:47 UTC by Jaroslav Tulach
Modified: 2012-03-23 19:25 UTC (History)
10 users (show)

See Also:
Issue Type: DEFECT
Exception Reporter:

Compatible patches. (293.73 KB, application/zip)
2010-03-25 15:03 UTC, Jan Lahoda
Incompatible patches. (293.42 KB, application/zip)
2010-03-25 15:03 UTC, Jan Lahoda
Convenience constructor (3.24 KB, patch)
2010-04-06 08:06 UTC, Jaroslav Tulach
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jaroslav Tulach 2010-02-09 01:47:03 UTC
The RequestProcessor.getDefault() is dangerous. From time to time I find profiling snapshots that contain many RP threads doing the same operation. Just yesterday I found about 40 threads in parsing.api:

Looks like almost all usages of RequestProcessor.getDefault() are wrong. Let's deprecated it. Jan Lahoda promised to help me with that:

Jan will create a Java Hint to automatically rewrite the improper use as I did manually in #3596ca474291.

Jan will apply the hint to whole NetBeans code base and attach it here. 

I will deprecate the method and update javadoc and apichanges.

After a week of review I will apply both patches. Affected code shall be reviewed by appropriate team and adjusted if necessary (by changing the throughput - not too common imho).
Comment 1 Jesse Glick 2010-02-09 11:19:22 UTC
(In reply to comment #0)
> The RequestProcessor.getDefault() is dangerous. [...]
> I will deprecate the method [...]

I don't think this is a good idea. There is nothing inherently harmful about using this method. If you expect some operation to be called dozens of times, then you should indeed read the Javadoc and make your own RP. But for a one-off action RP.default is fine: it is unlikely to be called again before the first completes, and even if so it is correct to run the second instance in parallel - excessive serialization of unrelated tasks can be harmful. You just need to look at the particular situation and decide what style of threading to use.

I would rather suggest adding some kind of run-time diagnostic when an unreasonably large number of RP threads are active simultaneously, especially if they all use a Runnable of the same impl class.
Comment 2 Jaroslav Tulach 2010-03-24 10:00:54 UTC
My experience of evaluating .nps files tells me that usage of RP.getDefault() is source of huge amount of errors. Anyway your idea of detecting that in runtime sounds good. Implemented as core-main#2bc794b05dce

Now we will see how often the RP.getDefault() is misused. I expect that soon the hint for converting to named RP will be really helpful.
Comment 3 Jesse Glick 2010-03-24 13:48:27 UTC
(In reply to comment #2)
> core-main#2bc794b05dce

final String msg = "Too many " + c.getName() + " in non-private processor. Use own static final RequestProcessor RP = new RequestProcessor(...)!"; // NOI18N
logger().log(Level.WARNING, msg, todo.item);

I just saw this reported today (for AU), but the exception reporter dialog only displayed the stack trace, not the msg - so it made no sense at all to me as a user: a $SlowItem with no message. See if you can get the exception reporter to display this properly.
Comment 4 Jan Lahoda 2010-03-25 15:02:31 UTC
Apparent P1, IMO - an unreviewed controversial incompatible API change (the RP.getDefault().post never threw/showed/logged exceptions before this).

I did a hint and applied to whole NB codebase+contrib:

Attaching two zips with patches: the compatible should not cause regressions (uses compatible throughput), incompatible can cause regressions (throughput == 1). Note the hint is not 100% tested (and not maintained), so make sure the changes build and work OK after you apply them. Also note that due to c120a96805ab, I was not able to module fix dependencies - I am working on a fix, but that is going to take some time and this apparently needed to be done very fast.

The patch was applied for:
java.editor java.freeform java.hints java.j2seplatform java.j2seproject java.navigation java.project java.source javadoc refactoring.api
Comment 5 Jan Lahoda 2010-03-25 15:03:14 UTC
Created attachment 95821 [details]
Compatible patches.
Comment 6 Jan Lahoda 2010-03-25 15:03:41 UTC
Created attachment 95822 [details]
Incompatible patches.
Comment 7 Jesse Glick 2010-03-25 15:24:55 UTC
[JG01] The compatible patches are pointless - they would just transfer the potentially poor performance of RP.default to a different form that is harder to search for.

I don't think it is wise to do a blanket automated conversion of all usages of RP.default. A human being needs to look at each case and decide whether unlimited (or otherwise high) throughput is appropriate for that case, or whether serialization is appropriate (and will not cause deadlocks or starvation of some kind). The usefulness of the recently introduced warning is that it points out places where high throughput is actually encountered in realistic usage and might need fixing.

[JG02] We should introduce a convenience ctor for RP taking Class rather than String so you could write just

private static final RequestProcessor RP =
  new RequestProcessor(JavaEditorWarmUpTask.class, 1, false, false);
Comment 8 Quality Engineering 2010-03-26 05:33:09 UTC
Integrated into 'main-golden', will be available in build *201003260201* on (upload may still be in progress)
User: Jesse Glick <>
Log: #180458: log only once per class per session.
Comment 9 Quality Engineering 2010-03-27 06:16:35 UTC
Integrated into 'main-golden', will be available in build *201003270201* on (upload may still be in progress)
User: Jan Lahoda <>
Log: #180458: not using RequestProcessor.getDefault() in java.editor java.freeform java.hints java.j2seplatform java.j2seproject java.navigation java.project java.source javadoc refactoring.api modules.
Comment 10 Jesse Glick 2010-03-31 20:55:39 UTC
Another case where using RP.default is fine (to my knowledge) is if you only use it to create(...) a single task, which you then schedule(...) when necessary. This should not trigger the runtime warning.
Comment 11 Jesse Glick 2010-03-31 21:03:14 UTC
(In reply to comment #10)
> if you only use it to create(...) a single task

(That is, really a single task for this class, not one per instance of the class, unless the class is a singleton.)
Comment 12 Jaroslav Tulach 2010-04-06 08:06:41 UTC
Created attachment 96732 [details]
Convenience constructor

I can hear a request for convenience constructor. Anything else that shall be done?
Comment 13 Geertjan Wielenga 2010-04-09 13:43:52 UTC
Yes, you should blog about it -- explain how RequestProcessor.getDefault works, why it is wrong, and how it should be done instead.
Comment 14 Jaroslav Tulach 2010-04-11 11:40:43 UTC
I will integrate the convenience constructor tomorrow and also extend the javadoc to describe what Geertjan requested (blog is not suitable place for documenting API, imho javadoc is better).
Comment 15 Geertjan Wielenga 2010-04-11 13:24:44 UTC
For the record: I have never said that the right place to document an API is in a blog. (However, if you're not going to blog about important changes to an API, the change is less likely to be noticed.)
Comment 16 Jaroslav Tulach 2010-04-12 05:30:25 UTC
core-main#625d6a876c6e - language fixes appreciated. Blogosphere publicity too.
Comment 17 Quality Engineering 2010-04-13 17:39:31 UTC
Integrated into 'main-golden', will be available in build *201004131450* on (upload may still be in progress)
User: Jaroslav Tulach <>
Log: #180458: Prevent parallel misuse of RequestProcessor.getDefault: Offer easy to use convenience constructor.
Comment 18 Jesse Glick 2012-03-23 19:25:41 UTC
I think the warnParallel=3 limit needs to be reconsidered. Since this change, quite a lot of bug reports have been produced by this stack trace, resulting in a significant amount of work to replace old calls to RP.gD with custom RP's. But in my experience hardly any of these cases involved any real performance problem. Rather, some user-initiated action needs something to be replanned into an RP thread, and on rare occasions the user clicked some button three times or whatever. Running those three tasks in parallel would usually have been harmless.

Nor is serializing those tasks necessarily better for performance; if they are mostly blocking on something else (e.g. network), it can actually make responsiveness worse to use a custom RP(Class) with throughput=1.

More useful IMHO would be track when the total number of RP threads in the process which are in runnable mode (i.e. not just blocked on some lock), or blocked in local disk I/O, exceeds some threshold of reasonableness beyond which they are likely to all just be thrashing one another. In such a case, report an exception with a stack trace blaming whichever RP name has contributed the most parallel threads. This would catch the actual rare abuses like the initially reported overparallelization in parsing.api, or property change "firestorms" in some node class, etc.