[External] : Re: Future.resultNow / exceptionNow

[Eric Kolotyluk]

I am trying to follow all the narratives...

Can I conclude that resultNow() does not need to attach any failure
information because the assumption is the future succeeded? In particular,
the recommended best practice is something like

structuredExecutor.join();
completionHandler.throwIfFailed();
var completedResults = futureResults.map(Future::resultNow).toList();

therefore, it should not be possible for any Futures to have failed. Any
failures would have been detected in throwIfFailed(). Consequently, calling
resultNow() on a Future that fails is "because the API is being used
incorrectly in the first place." that Ron was talking about?

While I can agree with all of this semantically, what concerns me is the
complexity, and the potential unnecessary cognitive load this places on
developers. This goes against my desire that Project Loom should be
intuitive, something that @Brian Goetz <brian.goetz at oracle.com> seemed to
support as a design goal.

Cheers, Eric

On Sun, Nov 21, 2021 at 8:54 AM Paul Bjorkstrand <paul.bjorkstrand at gmail.com>
wrote:

> To me, it sounds like the ask is this:
>
> "If the Future completed with an exception, why would the exception not
> be attached as the cause of the ISE thrown by resultNow()?"
>
> The answer Ron gave was:
>
> "The exception is not attached as the cause, because the API is being used
> incorrectly in the first place."
>
> I would argue that not attaching the cause is incorrect as well. I have
> seen many times in my career where catch blocks do not attach the cause
> when throwing again, which makes it rather difficult to track down the root
> problem. I agree that the method may have been used improperly, but I
> wouldn't want to hide the root cause.
>
> When you throw an exception *because of another exception* it is far better
> to add it as the cause, than to throw one with no cause. If you don't
> include the cause, you are hiding the root issue that needs to be solved.
> Yes, we may be using the API improperly, but are trying to teach people to
> use the API properly, or are you just trying to make it annoying if they
> _don't_ use it properly?
>
> I can't count how many times I have seen a catch-block throw a
> RuntimeException for a checked one, and didn't include the cause. In my
> book, that is a cardinal sin, akin to logging an exception like this in
> SLF4J:
> log.error("An exception happened: {}", e);
> Using either pattern, you lose the benefit of the stack trace and cause
> chain in the first place.
>
> For me, this goes beyond reducing boilerplate, and is more about good
> programming practices. The more information you can provide to the
> developer about what _exactly_ went wrong, the easier it is for that
> developer to find the issue.
>
> I agree, 100% with the other cases - task cancelled or thread interrupted
> or future not done yet - should throw as-is. All that is being asked for
> is, add the cause to the ISE when the future completed exceptionally. You
> will save people like me, hours of troubleshooting (and helping teach
> others to troubleshoot) errors that don't have useful stacktraces.
>
> -Paul
>
>
> On Sun, Nov 21, 2021 at 9:57 AM Alex Otenko <oleksandr.otenko at gmail.com>
> wrote:
>
> > On Sun, 21 Nov 2021, 14:56 Ron Pressler, <ron.pressler at oracle.com>
> wrote:
> >
> > >
> > >
> > > > On 21 Nov 2021, at 14:00, Alex Otenko <oleksandr.otenko at gmail.com>
> > > wrote:
> > > >
> > > > No, you don't know what went wrong. You only know something did.
> > >
> > > The methods must only be called when the Future is in a certain state,
> > and
> > > the exception tells you exactly what the actual, unexpected state is.
> You
> > > have all the information you could ever need to fix the bug. Suppose
> you
> > > call resultNow without having called join first. The error you’ll get
> > will
> > > tell you exactly that this is the bug, and that adding join is
> necessary.
> > > And suppose you call resultNow after join but before handling
> exception,
> > > the error you’ll get will tell you exactly that that’s what missing.
> > >
> >
> > No, because, to be precise, NPE un the task is the root cause. If not for
> > it, the task never throws, and in all the testing we never discovered the
> > missing exception handling in the code that calls resultNow.
> >
> > Now in production NPE got triggered. We look through the logs, and see
> one
> > bug, but not the other. To fix it, you need one patch just to make the
> NPE
> > discoverable. So you will lose time to resolution.
> >
> >
> >
> > > >
> > > > So, you see the code is broken. You get to deploy two patches: one to
> > > start using a different method to observe what has lead to the problem
> > > (likely a Runtime Exception or Error - say, NPE in the task), then the
> > fix
> > > for the actual problem.
> > >
> > > The exception tells you *which* incorrect state has led to the problem
> > > (maybe the message should include precisely which State enum value is
> > > expected and which is found).
> > >
> >
> > Not the full state, though, we learn just a single bit of information.
> >
> >
> >
> > > >
> > > > As for maybeResultNow - I haven't warmed up to the idea that throwing
> > by
> > > resultNow is saving me from a really bad problem.
> > >
> > > The problem that resultNow was created to solve is that in the common
> > > case, in which you already know the state of the future and have
> already
> > > handled exceptions, anything other than returning the result would be
> an
> > > unnecessary complication. Write down that code and see for yourself.
> > >
> >
> > Sure. Here it is:
> >
> > exe.join();
> > Throwable th = future.exceptionNow();
> > if (th != null) throw rewrap(th);
> > // here resultNow is guaranteed to be ok
> > // but exceptionNow will throw
> >
> >
> >
> > > >
> > > > I am looking at the code that joined on executor, and don't see why
> it
> > > is more correct for (albeit indirect) state introspection to punish me
> > with
> > > hidden throws.
> > >
> > > I don’t understand. The method for state introspection, Future.state,
> > > never fails. resultNow, which is *not* for state introspection, never
> > > punishes you. It just cannot continue when a bug in your program makes
> > its
> > > precondition false, and it helps you by telling you what your bug is.
> > >
> > > > I am looking at another example that polls for result, and again
> don't
> > > see why throwing is better for something that is like a non-blocking
> get.
> > >
> > > The purpose of resultNow is not to be a non-blocking get (although you
> > > could write a non-blocking get with a combination of state and
> > resultNow).
> > > The purpose of resultNow is to obtain the result in the common case, in
> > > which you already know that it’s there, and are *guaranteed* that the
> > > method will never fail.
> > >
> >
> > People will use it as a get() that doesn't require try-catch.
> >
> >
> > > > Reminds me of NIO Selector API, where almost anything can result in a
> > > bunch of Runtime Exceptions.
> > >
> > > If we could panic, maybe we’d panic. The exception is not the point. It
> > > just indicates a bug in the program.
> > >
> >
> > Well, and my contention is that you declare it a bug. What's the horrible
> > sin of the above check? A throwing exceptionNow doesn't really save me
> from
> > anything.
> >
> >
> > Alex
> >
> >
> > > I understand that you’d like a somewhat different feature, but please
> > > start with a motivation: take the current API and show what *problem
> you
> > > run into* that you’d like to solve.
> > >
> > > If your problem is unrelated to the JEP but is just about a
> non-blocking
> > > polling mechanism for Future, I’d still argue that having both state
> and
> > > resultNow is superior to a single maybeGetResult, because the former is
> > > more general: you can use it just as efficiently for polling or for
> > > obtaining the result in cases where the correct state is guaranteed.
> > >
> > > — Ron
> >
>