Allow blocking calls in WorkerTask#dispose
#3213
Conversation
This avoids flagging (short-lived) blocking calls that cannot always be avoided when a `dispose` call results in the removal of a task of the worker-backed queue when access to said queue is currently held by a guard. Fixes reactor#3210.
| @@ -74,26 +74,50 @@ public void shouldDetectBlockingCallsInOperators() { | |||
| } | |||
|
|
|||
| @Test | |||
| public void shouldNotReportScheduledFutureTask() { | |||
| for (int i = 0; i < 1_000; i++) { | |||
| public void shouldNotReportSchedulerScheduledFutureTask() { | |||
There was a problem hiding this comment.
I've adopted this test with learnings from writing the test below.
| } | ||
| finally { | ||
| taskScheduler.dispose(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| public void shouldNotReportWorkerScheduledFutureTask() { | ||
| for (int i = 0; i < 10_000; i++) { |
There was a problem hiding this comment.
On my local machine 10_000 was reliable in producing the exception -- 1_000 not so much. Let me know if this is acceptable.
There was a problem hiding this comment.
it does not have to be the same for CI env since it increases the general run time. Thus, let's keep it as it was before.
There was a problem hiding this comment.
As a follow up to this comment, I created #3216 which would create an opportunity to use a more heavy crunching when run locally and do a quicker run-through via CI.
| }; | ||
|
|
||
| for (int j = 0; j < 257; j++) { | ||
| worker.schedule(dummyRunnable, j + 1, TimeUnit.MILLISECONDS); |
There was a problem hiding this comment.
Since the race was between dispose (and consequently, queue#remove) and the queue picking up another task, I found that having alternate scheduling increased the likelihood of hitting the issue.
| } | ||
| Disposable disposable = worker.schedule(dummyRunnable, 1, TimeUnit.SECONDS); | ||
|
|
||
| RaceTestUtils.race(Schedulers.parallel(), disposable::dispose); |
There was a problem hiding this comment.
RaceTestUtils#race was refactored at some point to default to the bounded elastic scheduler on which blocking is "allowed", making the above test previously moot.
I use the RaceTestUtils even though we're not racing anymore as it's convenient for grabbing/throwing the exceptions.
|
@chemicL this PR seems to have been merged on a maintenance branch, please ensure the change is merge-forwarded to intermediate maintenance branches and up to |
| } | ||
| finally { | ||
| taskScheduler.dispose(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| public void shouldNotReportWorkerScheduledFutureTask() { | ||
| for (int i = 0; i < 10_000; i++) { |
There was a problem hiding this comment.
it does not have to be the same for CI env since it increases the general run time. Thus, let's keep it as it was before.
This avoids flagging (short-lived) blocking calls that cannot always be avoided when a
disposecall results in the removal of a task of the worker-backed queue when access to said queue is currently held by a guard.Fixes #3210.