Skip to content

Add @Nullable in RetryContext to easier detect possible NPE#457

Merged
artembilan merged 3 commits into
spring-projects:mainfrom
szpak-random-forks:nullableInRetryContext
Jul 25, 2024
Merged

Add @Nullable in RetryContext to easier detect possible NPE#457
artembilan merged 3 commits into
spring-projects:mainfrom
szpak-random-forks:nullableInRetryContext

Conversation

@szpak

@szpak szpak commented Jul 24, 2024

Copy link
Copy Markdown
Contributor

Both getParent() and getLastThrowable() might return null, as mentioned in javadoc. @Nullable helps an IDE warns developers about potential NPE.

It is a reminiscence of the problem, I've encountered recently, incorrectly assuming that onSuccess will only be executed if there was at least failed attempt before (which resulted in no throwable and NPE).

@szpak

szpak commented Jul 24, 2024

Copy link
Copy Markdown
Contributor Author

Btw, not strictly related to the PR, but re-reading javadoc:


	/**
	 * Accessor for the exception object that caused the current retry.
	 * @return the last exception that caused a retry, or possibly null. It will be null
	 * if this is the first attempt, but also if the enclosing policy decides not to
	 * provide it (e.g. because of concerns about memory usage).
	 */
	Throwable getLastThrowable();

I wonder, if "It will be null if this is the first attempt" is fully correct. In onError(), in the first attempt, it should be set. Only in onSuccess() in the first (successful) attempt it will be null. WDYT?

@artembilan

Copy link
Copy Markdown
Member

The logic there is like this:

		T result = retryCallback.doWithRetry(context);
					doOnSuccessInterceptors(retryCallback, context, result);
					return result;
				}
				catch (Throwable e) {

					lastException = e;

					try {
						registerThrowable(retryPolicy, state, context, e);
					}
					catch (Exception ex) {
						throw new TerminatedRetryException("Could not register throwable", ex);
					}
					finally {
						doOnErrorInterceptors(retryCallback, context, e);
					}

So, onSuccess() might be called immediately, or after some number of retries.
The onError() is always called for any failed attempt.

Therefore I think Javadoc is correct: if we call service and it does not fail we immediately go to the onSuccess() and exit from retry logic.
The "first attemp" is exactly that T result = retryCallback.doWithRetry(context); when we just entered to the retry loop.

@szpak

szpak commented Jul 24, 2024

Copy link
Copy Markdown
Contributor Author

The "first attemp" is exactly that T result = retryCallback.doWithRetry(context); when we just entered to the retry loop.

After NPE in the tests, I analyzed the code and draw similar conclusions :-). However, I would phrase it rather:

It will be null if this is the first attempt and it finishes successfully

or

It might be null in the first attempt (if it finishes successfully)

It emphasizes that only on success, it will be null (in the first failed attempt it should not be null - unless "the enclosing policy decides not to provide it").

Nevertheless, it's just wording and the current javadoc is pretty readable (if you read it first ;-) ).

@artembilan

Copy link
Copy Markdown
Member

Good. Feel free to update that Javadoc with your first suggestion as part of this PR and I’ll merge ASAP.

szpak added 2 commits July 24, 2024 23:31
Both getParent() and getLastThrowable() might return null, as mentioned
in javadoc. @nullable helps an IDE warns developers about potential NPE.
@szpak szpak force-pushed the nullableInRetryContext branch from fcfe975 to 67996be Compare July 24, 2024 21:34
@szpak

szpak commented Jul 24, 2024

Copy link
Copy Markdown
Contributor Author

Good. Feel free to update that Javadoc with your first suggestion as part of this PR and I’ll merge ASAP.

Done. I also amended the original commit to "fix" the "Unverified" signature warning.

@artembilan artembilan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error:  Failed to execute goal io.spring.javaformat:spring-javaformat-maven-plugin:0.0.41:validate (default) on project spring-retry: Formatting violations found in the following files:
Error:   * /home/runner/work/spring-retry/spring-retry/src/main/java/org/springframework/retry/RetryContext.java

Please, consider to run this Maven goal locally: spring-javaformat:apply.
And then push the changes.

Also add your name to the @author list of the modified class.

Thanks

@szpak

szpak commented Jul 25, 2024

Copy link
Copy Markdown
Contributor Author

I've just pushed the improved version.

@artembilan artembilan added this to the 2.0.8 milestone Jul 25, 2024
@artembilan artembilan merged commit 9df8d6c into spring-projects:main Jul 25, 2024
@artembilan

Copy link
Copy Markdown
Member

@szpak ,

thank you for contribution; looking forward for more!

@szpak szpak deleted the nullableInRetryContext branch July 25, 2024 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants