Comment 1 Miles Parker 2013-06-12 12:29:23 EDT

(In reply to comment #0)
> Steps:
> 1. Connect as anonymous
> 2. Create a query to bring in all reviews
> 
> Caused by: java.lang.NullPointerException
> at
> org.eclipse.mylyn.internal.gerrit.core.remote.GerritReviewRemoteFactory.updateApprovalsAndRequirements(GerritReviewRemoteFactory.java:261)
> at
> org.eclipse.mylyn.internal.gerrit.core.remote.GerritReviewRemoteFactory.updateModel(GerritReviewRemoteFactory.java:193)
> at
> org.eclipse.mylyn.internal.gerrit.core.remote.GerritReviewRemoteFactory.updateModel(GerritReviewRemoteFactory.java:1)
> at
> org.eclipse.mylyn.reviews.core.spi.remote.emf.RemoteEmfConsumer.applyModel(RemoteEmfConsumer.java:234)
> at
> org.eclipse.mylyn.reviews.core.spi.remote.JobRemoteService$3.run(JobRemoteService.java:100)
> at
> org.eclipse.ui.internal.UILockListener.doPendingWork(UILockListener.java:164)
> at org.eclipse.ui.internal.UISynchronizer$3.run(UISynchronizer.java:158)
> at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35)
> at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:135)
> ... 23 more
> 
> If I synchronize tasks that never finishes:
> 
> "Worker-3" prio=5 tid=0x000000010200b000 nid=0x8d03 waiting on condition
> [0x0000000113ff7000]
> java.lang.Thread.State: TIMED_WAITING (sleeping)
> at java.lang.Thread.sleep(Native Method)
> at
> org.eclipse.mylyn.internal.gerrit.core.GerritTaskDataHandler.updateModelData(GerritTaskDataHandler.java:147)
> at
> org.eclipse.mylyn.internal.gerrit.core.GerritTaskDataHandler.getTaskData(GerritTaskDataHandler.java:107)
> at
> org.eclipse.mylyn.internal.gerrit.core.GerritConnector.getTaskData(GerritConnector.java:160)
> at
> org.eclipse.mylyn.internal.tasks.core.sync.SynchronizeTasksJob.synchronizeTask(SynchronizeTasksJob.java:245)
> at
> org.eclipse.mylyn.internal.tasks.core.sync.SynchronizeTasksJob.runInternal(SynchronizeTasksJob.java:218)
> at
> org.eclipse.mylyn.internal.tasks.core.sync.SynchronizeTasksJob.run(SynchronizeTasksJob.java:153)
> at
> org.eclipse.mylyn.internal.tasks.core.sync.SynchronizeTasksJob.run(SynchronizeTasksJob.java:135)
> at
> org.eclipse.mylyn.internal.tasks.core.sync.SynchronizeQueriesJob.run(SynchronizeQueriesJob.java:225)
> at org.eclipse.core.internal.jobs.Worker.run(Worker.java:53)

Does cancelling work? I initially had some timeout code in there, but then thought it better to just respect the user cancel, but if that isn't working we need a seperate bug for this.

Comment 2 Miles Parker 2013-06-12 14:24:01 EDT

(In reply to comment #0)
> Steps:
> 1. Connect as anonymous
> 2. Create a query to bring in all reviews
> 
> Caused by: java.lang.NullPointerException
> at
> org.eclipse.mylyn.internal.gerrit.core.remote.GerritReviewRemoteFactory.updateApprovalsAndRequirements(GerritReviewRemoteFactory.java:261)
> at

Hmm... I'm not able to reproduce. The code does assume that you have a configuration. I wonder if that's a faulty assumption. Do you get the "Retrieving Configuration" message when you first create it? What's the exact query you're using? (I tried All Open.)

Comment 3 Steffen Pingel 2013-06-12 14:43:05 EDT

Canceling worked but the editor was not rendering properly since it never retrieved the model data.

I wasn't able to retrieve configuration. I assume that is what caused the subsequent failures.

Comment 4 Miles Parker 2013-06-12 14:47:25 EDT

(In reply to comment #3)
> Canceling worked but the editor was not rendering properly since it never
> retrieved the model data.

Right.

> I wasn't able to retrieve configuration. I assume that is what caused the
> subsequent failures.

Yes, exactly. It sounds like not being able to retrive the configuration is the real issue. I don't think it makes sense to guard against not having a configuration, since there is so much code that (reasonably, I think) relies on that.

Comment 5 Steffen Pingel 2013-06-12 15:21:07 EDT

(In reply to comment #4)
> Yes, exactly. It sounds like not being able to retrive the configuration is the
> real issue. I don't think it makes sense to guard against not having a
> configuration, since there is so much code that (reasonably, I think) relies on
> that.

All code that relies on configuration needs to retrieve it in case it's not cached. Configuration data could get lost for any reason so code should never just assume that it's there (maybe none of the code does that I haven't checked).

Comment 6 Miles Parker 2013-06-12 15:30:17 EDT

(In reply to comment #5)
> All code that relies on configuration needs to retrieve it in case it's not
> cached. Configuration data could get lost for any reason so code should never
> just assume that it's there (maybe none of the code does that I haven't
> checked).

I checked before my last comment, and I don't think any of the ~13 usages check for it, but perhaps there are some sort of higher level checks that I'm missing. (Just to mention, I'm pretty sure this was true of 1.1 as well.. )

Comment 7 Miles Parker 2013-06-17 19:02:05 EDT

So, we do already do a GerritClient.refreshConfigOnce(new NullProgressMonitor()); every time before we do a review pull. The only thing I can think is that somehow the configuration went stale between the time we pulled and the time we tried to grab the other approvals. Or perhaps we should be forcing the config to refresh?

I'm still unable to reproduce by changing config to anyonmous access, even after restarting, so it's difficult to determine for sure if that was the issue. Does it continue to fail after that query or does this just happen once?

Comment 8 Miles Parker 2013-06-17 20:14:49 EDT

https://git.eclipse.org/r/#/c/13861

I think the issue was much simpler, actually. I don't the NPE was in the configuration at all, but in the Approvals, which should be non-existent for the case of anonymous. So we just needed some more null checks here.

Comment 9 Miles Parker 2013-06-17 20:36:08 EDT

Merged https://git.eclipse.org/r/#/c/13861

Steffen, please reopen if you see this issue again.

Comment 10 Tomasz Zarna 2013-07-05 12:28:34 EDT

I ain't Steffen, but I just saw this. Created a query for all open reviews as anonymous, against a 2.5.4 repo. Running of master.

org.eclipse.swt.SWTException: Failed to execute runnable (java.lang.NullPointerException)
	at org.eclipse.swt.SWT.error(SWT.java:4361)
	at org.eclipse.swt.SWT.error(SWT.java:4276)
	at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:138)
	at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:4144)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3761)
	at org.eclipse.ui.internal.Workbench.runEventLoop(Workbench.java:2701)
	at org.eclipse.ui.internal.Workbench.runUI(Workbench.java:2665)
	at org.eclipse.ui.internal.Workbench.access$4(Workbench.java:2499)
	at org.eclipse.ui.internal.Workbench$7.run(Workbench.java:679)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:332)
	at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:668)
	at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:149)
	at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:124)
	at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:196)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:110)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:79)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:353)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:180)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:601)
	at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:629)
	at org.eclipse.equinox.launcher.Main.basicRun(Main.java:584)
	at org.eclipse.equinox.launcher.Main.run(Main.java:1438)
	at org.eclipse.equinox.launcher.Main.main(Main.java:1414)
Caused by: java.lang.NullPointerException
	at org.eclipse.mylyn.internal.gerrit.core.remote.GerritReviewRemoteFactory.updateModel(GerritReviewRemoteFactory.java:198)
	at org.eclipse.mylyn.internal.gerrit.core.remote.GerritReviewRemoteFactory.updateModel(GerritReviewRemoteFactory.java:1)
	at org.eclipse.mylyn.reviews.core.spi.remote.emf.RemoteEmfConsumer.applyModel(RemoteEmfConsumer.java:234)
	at org.eclipse.mylyn.reviews.core.spi.remote.JobRemoteService$3.run(JobRemoteService.java:95)
	at org.eclipse.ui.internal.UILockListener.doPendingWork(UILockListener.java:164)
	at org.eclipse.ui.internal.UISynchronizer$3.run(UISynchronizer.java:158)
	at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35)
	at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:135)
	... 23 more

Comment 11 Tomasz Zarna 2013-07-05 12:36:07 EDT

(In reply to comment #10)
> Caused by: java.lang.NullPointerException
> 	at
> org.eclipse.mylyn.internal.gerrit.core.remote.GerritReviewRemoteFactory.
> updateModel(GerritReviewRemoteFactory.java:198)

GerritConfiguration is null as Anonymous cannot read the config, see bug 412102 comment 1

Comment 12 Tomasz Zarna 2013-07-08 06:52:17 EDT

See also bug 412496, for an exception thrown when adding a comment as anonymous user.

Comment 13 Miles Parker 2013-07-11 13:08:13 EDT

https://git.eclipse.org/r/#/c/14454 or similar solution should address this.

Comment 14 Steffen Pingel 2013-07-12 13:43:54 EDT

The review does not address the problem. My guess is that the configuration is null because it hasn't been cached, yet. You should be able to reproduce that by shutting down Eclipse, deleting the configuration cache and restarting.

Comment 15 Miles Parker 2013-07-12 17:57:50 EDT

(In reply to comment #14)
> The review does not address the problem. My guess is that the configuration
> is null because it hasn't been cached, yet. You should be able to reproduce
> that by shutting down Eclipse, deleting the configuration cache and
> restarting.

I think that you can't get the configuration (it's really confusing because there are two if you include the Mylyn GerritConfiguration, here I'm referring to the GerritConfig) at all in this case. I don't have the code in front of me (not at work today), but I think that's a central assumption of the change. But I could be wrong about that.

Comment 16 Steffen Pingel 2013-07-12 18:31:10 EDT

There are two scenarios:

1. The client has cached the configuration either in this session or a previous session. Both GerritConfiguration and GerritConfig are available.
2. The client has never cached the configuration because refreshConfigOnce() was not invoked or downloading of the configuration failed.

Any operation that relies on configuration (e.g. for approvals) needs to trigger a configuration download in scenario (2). If downloading fails the repository is not functional and the operation needs to fail.

The scenario where only GerritConfiguration but not GerritConfig exists doesn't make sense, they have a 1:1 entity relationship.

Problems could arise if the configuration was cached and gets lost (for whatever reason) and UI elements rely on it being there but can't refresh it from the UI thread or because a connection can't be established. That's the scenario that we need to look at and handle gracefully (so may actually need some null checks for GerritConfiguration but not GerritConfig).

Comment 17 Miles Parker 2013-07-15 13:02:16 EDT

Ok, where does Account fit into that? Because I don't think it's possible to have an Account without a valid login, is it? And it seemed that you couldn't get a GerritConfig either, but I could be wrong about that.

Comment 18 Miles Parker 2013-07-15 13:32:46 EDT

See bug 410597, I think Steffen is saying that the deeper issue is that we don't have a configuration and we should, not that we aren't properly checking for lack of configuration. I think we need to discuss how these two fit together.

Comment 19 Miles Parker 2013-07-15 14:38:29 EDT

Also note that this change handles one other issue. It supresses the user specific searches when we have an anonymous user.  See:

https://git.eclipse.org/r/#/c/14294/12/org.eclipse.mylyn.gerrit.ui/src/org/eclipse/mylyn/internal/gerrit/ui/wizards/GerritCustomQueryPage.java

Please let me know if you'd like me to break any of these out into seperate reviews. :)

Comment 20 Steffen Pingel 2013-07-15 16:22:15 EDT

(In reply to comment #19)
> Also note that this change handles one other issue. It supresses the user
> specific searches when we have an anonymous user.  See:
> 
> https://git.eclipse.org/r/#/c/14294/12/org.eclipse.mylyn.gerrit.ui/src/org/eclipse/mylyn/internal/gerrit/ui/wizards/GerritCustomQueryPage.java
> 
> Please let me know if you'd like me to break any of these out into seperate
> reviews. :)

That has always been broken as far as I know. I don't think hiding the UI elements is the a good solution though since it's not obvious what's going on. I'd indeed prefer if you could file a separate bug (if we don't have one already) and split out the change.

Comment 21 Miles Parker 2013-07-15 17:08:30 EDT

(In reply to comment #20)
> That has always been broken as far as I know. I don't think hiding the UI
> elements is the a good solution though since it's not obvious what's going on.

Good point.

> I'd indeed prefer if you could file a separate bug (if we don't have one
> already) and split out the change.

bug 413010 (It doesn't make sense to split this out until we know how we're going to handle anonymous configurations.

Comment 22 Eclipse Webmaster 2022-11-15 11:45:08 EST

Mylyn has been restructured, and our issue tracking has moved to GitHub [1].

We are closing ~14K Bugzilla issues to give the new team a fresh start. If you feel that this issue is still relevant, please create a new one on GitHub.

[1] https://github.com/orgs/eclipse-mylyn