Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

org.eclipse.core.tests.runtime.jobs.JobEventTest | testSleepOrderOfEvents fails on windows intermittently #271

Closed
ktatavarthi opened this issue Nov 18, 2022 · 9 comments · Fixed by #280
Closed

org.eclipse.core.tests.runtime.jobs.JobEventTest | testSleepOrderOfEvents fails on windows intermittently #271

ktatavarthi opened this issue Nov 18, 2022 · 9 comments · Fixed by #280
Labels
bug Something isn't working regression Regression defect

Comments

@ktatavarthi
Copy link
Member

org.eclipse.core.tests.runtime.jobs.JobEventTest | testSleepOrderOfEvents failed on windows in the build I20221117-1330 with the below error

java.lang.AssertionError: Race condition at Event 7 Job.run() in Thread 'Worker-659: testSleepOrderOfEvents'

java.lang.AssertionError: java.lang.AssertionError: Race condition at Event 7 Job.run() in Thread 'Worker-659: testSleepOrderOfEvents'
at org.eclipse.core.tests.runtime.jobs.OrderAsserter.lambda$0(OrderAsserter.java:143)
at java.base/java.util.concurrent.ConcurrentLinkedQueue.forEachFrom(ConcurrentLinkedQueue.java:1037)
at java.base/java.util.concurrent.ConcurrentLinkedQueue.forEach(ConcurrentLinkedQueue.java:1054)
at org.eclipse.core.tests.runtime.jobs.OrderAsserter.assertNoErrors(OrderAsserter.java:142)
at org.eclipse.core.tests.runtime.jobs.JobEventTest.testSleepOrderOfEvents(JobEventTest.java:315)
@ktatavarthi ktatavarthi mentioned this issue Nov 18, 2022
Closed
13 tasks
@iloveeclipse
Copy link
Member

@jukzi : could you please take a look?

@jukzi
Copy link
Contributor

jukzi commented Nov 18, 2022

The given link does not show that error which makes it impossible to analyze.

@iloveeclipse
Copy link
Member

The given link does not show that error which makes it impossible to analyze.

I also saw this fail this morning.
See eclipse-platform/eclipse.platform.releng.aggregator#674 (comment)

@iloveeclipse
Copy link
Member

OK, the lost test fail can be seen here:
https://ci.eclipse.org/releng/job/AutomatedTests/job/ep426I-unit-win32-java11/33/#showFailuresLink

https://ci.eclipse.org/releng/job/AutomatedTests/job/ep426I-unit-win32-java11/33/testReport/junit/org.eclipse.core.tests.runtime.jobs/JobEventTest/testSleepOrderOfEvents/

@jukzi
Copy link
Contributor

jukzi commented Nov 21, 2022

Race condition at Event 7 Job.run() in Thread 'Worker-659: testSleepOrderOfEvents' with Event 6 IJobChangeEvent.running() in Thread 'Worker-660'

Caused by: java.lang.AssertionError: Race condition at Event 7 Job.run() in Thread 'Worker-659: testSleepOrderOfEvents'
	at org.eclipse.core.tests.runtime.jobs.OrderAsserter.addError(OrderAsserter.java:127)
	at org.eclipse.core.tests.runtime.jobs.OrderAsserter.expect(OrderAsserter.java:101)
	at org.eclipse.core.tests.runtime.jobs.OrderAsserter.expect(OrderAsserter.java:68)
	at org.eclipse.core.tests.runtime.jobs.JobEventTest$5.run(JobEventTest.java:259)
	at org.eclipse.core.internal.jobs.Worker.run(Worker.java:63)
Caused by: java.lang.AssertionError: Race condition at Event 7 Job.run()
	at org.eclipse.core.tests.runtime.jobs.OrderAsserter.expect(OrderAsserter.java:84)
	... 3 more
Caused by: java.lang.IllegalStateException: Race condition with Event 6 IJobChangeEvent.running() in Thread 'Worker-660'
	at org.eclipse.core.tests.runtime.jobs.OrderAsserter.expect(OrderAsserter.java:89)
	at org.eclipse.core.tests.runtime.jobs.OrderAsserter.expect(OrderAsserter.java:68)
	at org.eclipse.core.tests.runtime.jobs.JobEventTest$6.running(JobEventTest.java:298)
	at org.eclipse.core.internal.jobs.JobListeners.sendEvent(JobListeners.java:138)
	at org.eclipse.core.internal.jobs.JobListeners.sendEventsAsync(JobListeners.java:118)
	at org.eclipse.core.internal.jobs.JobListeners.waitAndSendEvents(JobListeners.java:78)
	at org.eclipse.core.internal.jobs.JobManager.startJob(JobManager.java:1752)
	at org.eclipse.core.internal.jobs.WorkerPool.startJob(WorkerPool.java:244)
	at org.eclipse.core.internal.jobs.Worker.run(Worker.java:58)

Indeed currentJob.run is called without waiting for all events to be processed first. If another worker process processes the event the job.eventQueue.isEmpty() check in waitAndSendEvents will be true before the event was processed.

jukzi pushed a commit to jukzi/eclipse.platform that referenced this issue Nov 21, 2022
@EcljpseB0T
fix race condition Job.run() with IJobChangeEvent.running() eclipse-p…
9786af2 
…latform#271

The abbreviation check eventQueueThread.isEmpty() can become true before
all events processed. Need to have a lock before doing the check.
@jukzi jukzi linked a pull request Nov 21, 2022 that will close this issue
fix race condition Job.run() with IJobChangeEvent.running() #271 #280
Merged
@iloveeclipse iloveeclipse added bug Something isn't working regression Regression defect labels Nov 21, 2022
@iloveeclipse
Copy link
Member

Regression seem to exist since #193, or is this is a regression from #263?

@jukzi : please could you assess the severity of the issue.

What could be possible regressions from that?

  • Missing events?
  • Event sent in wrong order?
  • Race conditions in Job framework?

and with that, please assess possible criticality of the proposed patch.

The main question for me is: if that should be added to RC2, as a regression in Job framework?

@akurtakov: FYI.

@akurtakov
Copy link
Member

Do we have an issue for real breakage somewhere? If not I would say this is not for RC2 as the change is in quite critical place.

@jukzi
Copy link
Contributor

jukzi commented Nov 21, 2022

It could happen is that IJobChangeEvent.running() and Job.run() are executed in parallel. It was never guaranteed in the API that they would not do so. But since the historic implementation did always executed running() before run() there might be clients that relied on that behaviour even though they should not.
I am not aware of any serious bad thing that could happen in the IDE.

akurtakov pushed a commit to jukzi/eclipse.platform that referenced this issue Nov 22, 2022
@EcljpseB0T @akurtakov
fix race condition Job.run() with IJobChangeEvent.running() eclipse-p…
3c79b75 
…latform#271

The abbreviation check eventQueueThread.isEmpty() can become true before
all events processed. Need to have a lock before doing the check.
iloveeclipse pushed a commit to jukzi/eclipse.platform that referenced this issue Dec 13, 2022
@EcljpseB0T @iloveeclipse
fix race condition Job.run() with IJobChangeEvent.running() eclipse-p…
e29fbbd 
…latform#271

The abbreviation check eventQueueThread.isEmpty() can become true before
all events processed. Need to have a lock before doing the check.
@iloveeclipse
Copy link
Member

Failed again in https://download.eclipse.org/eclipse/downloads/drops4/I20221212-1800/testresults/html/org.eclipse.core.tests.runtime_ep427I-unit-cen64-gtk3-java17_linux.gtk.x86_64_17.html

java.lang.AssertionError: Race condition at Event 7 Job.run() in Thread 'Worker-586: testSleepOrderOfEvents'

java.lang.AssertionError: java.lang.AssertionError: Race condition at Event 7 Job.run() in Thread 'Worker-586: testSleepOrderOfEvents'
at org.eclipse.core.tests.runtime.jobs.OrderAsserter.lambda$0(OrderAsserter.java:143)
at java.base/java.util.concurrent.ConcurrentLinkedQueue.forEachFrom(ConcurrentLinkedQueue.java:1037)
at java.base/java.util.concurrent.ConcurrentLinkedQueue.forEach(ConcurrentLinkedQueue.java:1054)
at org.eclipse.core.tests.runtime.jobs.OrderAsserter.assertNoErrors(OrderAsserter.java:142)
at org.eclipse.core.tests.runtime.jobs.JobEventTest.testSleepOrderOfEvents(JobEventTest.java:315)

jukzi pushed a commit to jukzi/eclipse.platform that referenced this issue Jan 9, 2023
@EcljpseB0T
fix race condition Job.run() with IJobChangeEvent.running() eclipse-p…
5755d4a 
…latform#271

The abbreviation check eventQueueThread.isEmpty() can become true before
all events processed. Need to have a lock before doing the check.
@jukzi jukzi closed this as completed in #280 Jan 9, 2023
jukzi pushed a commit that referenced this issue Jan 9, 2023
@EcljpseB0T @jukzi
fix race condition Job.run() with IJobChangeEvent.running() #271
2f1fbac 
The abbreviation check eventQueueThread.isEmpty() can become true before
all events processed. Need to have a lock before doing the check.
@fedejeanne fedejeanne mentioned this issue Jul 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working regression Regression defect
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix race condition Job.run() with IJobChangeEvent.running() #271 jukzi/eclipse.platform
4 participants
@akurtakov @iloveeclipse @ktatavarthi @jukzi