Always inject FinishedJobs instance into ProgressServiceImpl #1887

[selundqma]

The "finishedJobs" variable isn´t injected, thus is always null, when used from a progress dialog. Since the @Optional field in combination with @Inject only refers to already created instances of FinishedJobs. But, this does not happen in this flow.

Fixes https://github.com/eclipse-platform/eclipse.platform.ui/issues/1887

[github-actions[bot]]

Test Results

 1 812 files  +  604   1 812 suites  +604   1h 32m 59s :stopwatch: + 40m 7s  7 614 tests ±    0   7 386 :white_check_mark: ±    0  228 :zzz: ±  0  0 :x: ±0  23 994 runs  +7 998  23 245 :white_check_mark: +7 718  749 :zzz: +280  0 :x: ±0 

Results for commit 063a2faf. ± Comparison against base commit 6990a01b.

:recycle: This comment has been updated with latest results.

[fedejeanne]

Hello @selundqma , thank you for contributing this!

Could you please provide a snippet to reproduce the error (even better, a test)? I wonder if removing the Optional annotation is the right approach since all other fields in the class are also marked optional but they are being injected, right?

Can you describe the behavior more in detail? Are the other fields (always) being injected? Are there some use cases where finishedJobs is still being injected even before this PR (i.e. still marked @Optional)? The error may lay somewhere else perhaps?

[laeubi]

@fedejeanne The field will be filled if someone else requested the creation of the singelton, I think thats the original intend but I can't see it harms as the object is literally always created on all other places.

Beside that it might be just that the null case has to be handled properly as an alternative.

[fedejeanne]

@fedejeanne The field will be filled if someone else requested the creation of the singelton, I think thats the original intend but I can't see it harms as the object is literally always created on all other places.

What I understand from this is that it should be possible to request the creation of the FinishedJobs in this use case too, right? How can one achieve that?

@selundqma if you could provide a snippet to reproduce the error maybe we could analyze it and see if something is missing.

Beside that it might be just that the null case has to be handled properly as an alternative.

If that's the case then the other fields in the class should also be checked for null, right? It seems odd that none of these checks exists today and we haven't really seen many NPEs, so maybe there is another way? Again, a snippet would help (me) understand this issue a bit better.

[laeubi]

What I understand from this is that it should be possible to request the creation of the FinishedJobs in this use case too, right? How can one achieve that?

The Class is marked with @Creatable so it will be constructed when first requested.

[fedejeanne]

What I understand from this is that it should be possible to request the creation of the FinishedJobs in this use case too, right? How can one achieve that?

The Class is marked with @Creatable so it will be constructed when first requested.

If I'm reading correctly between the lines here, this means that the lazy initialization failed and the solution (removing @Optional) is actually a hack to force the initialization (which might be even unwanted in other use cases --> https://github.com/eclipse-platform/eclipse.platform.ui/issues/1887#issuecomment-2111798833). If that's the case then I think the solution proposed in this PR might be hiding the real problem, which is: creating the object under demand (@Optional) doesn't work as expected.

[selundqma]

Hi @fedejeanne and @laeubi!

I guess the person(s) who wrote the code is not available anymore to explain the reason for the use of @Optional?

It is hard for me to provide any code snippets due to company policies. In short we create a job (WorkspaceJob) and schedule it, implement the ProgressMonitor interface and use SubMonitor.convert() to handle multiple downloads in the progress dialog.

We also show the Progress View in our application and it is possible to cancel a single job in the Progress View since the view is created with the following method where FinishedJobs is injected without the @Optional attribute:

	@PostConstruct
	public void createPartControl(Composite parent, ProgressManager progressManager,
			IProgressService progressService, FinishedJobs finishedJobs,
			ProgressViewUpdater viewUpdater) {
		...
	}

The other fields are registered in different ways and I guess that is why it works fine with those two:

ProgressManager:
	public class ProgressViewAddon {
	...
		ProgressManager progressManager = ContextInjectionFactory.make(ProgressManager.class, context);
		appContext.set(ProgressManager.class, progressManager);
	..

ContentProviderFactory:
	@PostConstruct
	void init() {
		services.registerService(ContentProviderFactory.class, this);
	}

An observation: ProgressManager.java, ProgressRegion.java, ClearAllHandler.java and more do not use @Optional. Next observation: null is also present when calling services.getService(FinishedJobs.class) in ContentProviderFactory. I cannot see any place in the code where a FinishedJobs service is registered...

Since the FinishedJobs class is both @Creatable and @Singleton, it seems to be very low risk to remove the @Optional attribute in ProgressServiceImpl. There can only exist one instance. And when we don´t have one and need one, we should create it (as in the createPartControl() method in ProgressView.

[fedejeanne]

@selundqma thank you for your input. Can you explain why @Optional and @Inject doesn't work as expected?

[selundqma]

https://www.eclipse.org/forums/index.php?t=msg&th=488565&goto=1062955&#msg_1062955

[fedejeanne]

@selundqma thanks for the link, it explains it perfectly.

[selundqma]

Thank you for approving the PR! Just to understand the process here, what happens now? Are we waiting for more reviewers? Maybe you have "internal meetings" to decide what to merge or not? This is my first pull request for this project so I am a bit insecure how it all happens here.

[fedejeanne]

@selundqma once the freeze period is over, this PR can be merged. I happen to be out of office next week so I'll merge it in June.

[iloveeclipse]

Please don't merge anything to master until master is opened for 4.33!!!

[iloveeclipse]

@fedejeanne : once master opens, please bump bundle version for org.eclipse.e4.ui.progress bundle.

[fedejeanne]

@fedejeanne : once master opens, please bump bundle version for org.eclipse.e4.ui.progress bundle.

Sorry, I relied on the "check freeze period" (check), my mistake. I'll bump the version after master is reopened for development.