ModuleLayerHandler.getLayer throws NPE if layer is missing

[Johni0702]

modlauncher/src/main/java/cpw/mods/modlauncher/ModuleLayerHandler.java

Lines 86 to 88 in 9b69c82

	public Optional<ModuleLayer> getLayer(final Layer layer) {
	return Optional.ofNullable(completedLayers.get(layer).layer());
	}

Calling this method with a Layer that's not yet available throws a NullPointerException instead of returning an empty Optional as one might expect from its signature.

[marchermans]

I understand your case here, but when would this happen?

[marchermans]

@Johni0702 What are you doing to trigger this crash?

[Johni0702]

@Johni0702 What are you doing to trigger this crash?

Literally what I said:

Calling this method with a Layer that's not yet available

Launcher.INSTANCE.findLayerManager().flatMap(it -> it.getLayer(IModuleLayerManager.Layer.GAME).orElse(...)
called from various places, and I would expect those where the game layer is not yet available to safely fall back to the orElse case.

[marchermans]

But why, as I stated in the PR for this, we construct the layer as soon as we can, so what are you doing that you would want to access the game layer before it is even build?

Don't get me wrong I think we should fix this, but that does not mean I don't need more context.

[Johni0702]

It's complicated. I've got a modloader-independent code base that interacts with various modloader-specific sub-projects at various points in time during the mod loading process.
Yes, it wouldn't make a lot of sense to call that method at that specific point in the loading process if that's the only thing I ever did. But it gets called multiple times, and I'm expecting it to not be present for some of those calls (because they aren't primarily targeted at modlauncher), that's why I used an orElse rather than a orElseThrow.

[marchermans]

I understand that you are invoking it in such a context, just not why specifically?
Like what practical thing are you trying to achieve? Can you send me a link to the code so I can try to learn from your usecase?

[Johni0702]

As I said, it's complicated, and I do not wish to waste more time explaining it.

[marchermans]

I don't understand how this is wasting time.

If you want me to merge the fix in a way that I can be confident that it won't cause issues else where, and in a way that guarantees me that there are no issues like this else where, I need to know the context of the call.

I don't ask for your help with the research just a pointer to the code that is causing the issue, so that I can verify the problem and consider your use case in future PR merges or redesigns.

If you feel like you don't want to spend time helping me get a better understanding of the issue and its context, I don't need to spend the time reviewing, testing and verifying the pr which is ready to be merged and I am pretty sure nobody else will spend time on that either then.

[marchermans]

Why did you close this ticket?
It is a valid ticket... And the issue is still open, I just need more context to make a confident decision about what is going on.

[Johni0702]

Because this is a trivial issue with a dead simple fix and you're wasting an unreasonable amount of time on it.

[marchermans]

I totally understand your position, and it is indeed a trivial fix. But even trivial fixes can have far reaching consequences.
So I tend to make an effort when issues like this arise to understand the context they are effectively happening in, so that I can make a informed decision on how to deal with the accompanying Pull Request.

For example there are two primary ways we can deal with this issue:

1. The trivial solution proposed in Fix ModuleLayerHandler#getLayer NPE on missing layer #105, which I am happy to merge, since it solves the issue, once I get some context as to how and why this happens. Maybe there is a good reason this is called early? Maybe not. Regardless it needs to be dealt with.
2. Properly define the behavior in the case the initialization has not happened yet, but a request for it comes in. This can range from returning null, throwing an exception or apply the fix from point 1) here. But unless people (read you) show me as to what you are doing with this API, I can not make an informed decision on this, and then I just tend to pick the decision which produces the clearest results and might or might not help you along in your endeavors of modding minecraft.

Do you know understand why having the context is a necessary evil that I need to make a proper decision on this issue?
In the time we have discussed this today, it would have been easy for you to point me in the right direction and then I would likely just shut up and merge it, but now we have not gotten anywhere and the issue is still unresolved (and will likely stay that way, until somebody shows me the context in which this occurs) and the problem you are having is still happening to you, which is also not something I want.

[Johni0702]

Ok, I think I see were our difference in perception comes from:

I have not explicitly stated this, but I am not at all suggesting that the (implied) API should be in any way changed. The way this method is currently defined (with an Optional<ModuleLayer> return type) is in my opinion perfectly fine, coherent with the remainder of the ModLauncher APIs and would work perfectly fine for my use case. An Optional is a perfectly reasonable way to communicate simple failures to the caller so they can handle the error in whatever way is most appropriate for their use case.

To me this looks like a straight forward bug in the implementation which simply doesn't match the implied API. If a layer is missing, I'm expecting it to return Optional.empty (why else would it return an Optional) and it seems that this even was the intention of whoever wrote the implementation (cause they did use Optional.ofNullable, not just Optional.of), they just got the details wrong. Easy mistake to make, easy to fix.

Note how for none of this it actually matters why getLayer is called. I wasn't questioning the API design itself (where the actual use case does matter), just the apparent disconnect between heavily implied behavior (both in API and in attempted implementation) and actual behavior (throwing a NPE, which is totally unexpected).

Maybe there is a good reason this is called early? Maybe not. Regardless it needs to be dealt with.

There is a good reason in my case (I attempted to explain that in short in one of my replies above). In any case, the current API already deals with it, that's part of the motivation for why Optional exists in the first place, to force you to consider how you wish to handle both cases. If you're not expecting the empty case, you can simply orElseThrow() and it'll still fail fast for you. But if you do have a good way to handle it, then you're free to go down that route as well.

[marchermans]

I understand your position just fine and there was no difference in perception. Just a difference in how we deal with issues like this.
Yes this is a bug, but every time we fix bugs, we need to asks ourselves if the API surface is fine the way it is or not.

Now currently that optional is likely there for a reason so I think the fix is appropriate, but I would just like to know how you use this API practically, maybe there is something we can do to get you a better API besides just fixing the bug, and helping you make your code better understandable by making the API better.

However if you don't want to show me your code, then say so, and I will see what I can do in the mean time.

[Johni0702]

As I've said, the API is perfectly fine for my use case. If I wanted a better API for what I'm doing, I would have opened an issue asking for one.
I cannot share the code in question, nor do I want to because I have not come here to receive help for it. It works fine. The only reason I've opened this ticket is to save others from getting bitten by the same bug.

[marchermans]

Fine I will triage it again on this release then.