Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ensures SignalListener#addToContext exceptions are handled #3415

Merged
merged 1 commit into from
Apr 10, 2023

Conversation

chemicL
Copy link
Member

@chemicL chemicL commented Mar 24, 2023

Fixes #3414

@chemicL chemicL added type/bug A general bug area/context This issue is related to the Context labels Mar 24, 2023
@chemicL chemicL added this to the 3.5.5 milestone Mar 24, 2023
@chemicL chemicL requested a review from a team as a code owner March 24, 2023 10:56
@chemicL chemicL self-assigned this Mar 24, 2023
@@ -661,6 +712,47 @@ void doFirst() {
);
}

@Test
void throwingAlterContext() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess those duplicate tests could be implemented through Stream<Consumer[TestSubscriber]> so you pass subscriber and then check that it received onError

Copy link
Member Author

Choose a reason for hiding this comment

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

It's part of a @Nested test. I believe the original suite was meant to group by operator type (combinations of conditonal/fuseable/regular with flux/mono) and in a nested class have all that's appropriate at the cost of duplication. Do I understand correctly that you'd like to change the paradigm - have just doFirst, throwingAlterContext, throwingCreateListener, etc. that accept consumers and make their assertions as generic validations, but have the logic in the Stream of parameters that consume a TestSubscriber? If it can be done, perhaps it can be addressed in a different PR? Rewriting it here would blur the point this PR is making. Unless of course, I misunderstood your intention?

catch (Throwable e) {
IllegalStateException listenerError = new IllegalStateException(
"Unable to augment tap Context at subscription via addToContext", e);
signalListener.handleListenerError(listenerError);
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at this handler I wonder what if it throws exception as well

Copy link
Member Author

Choose a reason for hiding this comment

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

The javadoc says the following:

This method MUST return normally, i.e. it MUST NOT throw..

The original design assumes therefore, that ignoring this warning can lead to unspecified behaviour.

Copy link
Contributor

Choose a reason for hiding this comment

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

ahh. I see

Copy link
Contributor

@OlegDokuka OlegDokuka left a comment

Choose a reason for hiding this comment

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

LGTM with minor comments that could improve tests and another thought on wrapping handleListenerError since it may throw an exception as well

@OlegDokuka OlegDokuka changed the title Not ignoring SignalListener#addToContext exceptions ensures SignalListener#addToContext exceptions are handled Apr 10, 2023
@OlegDokuka OlegDokuka merged commit d31f1bc into main Apr 10, 2023
@OlegDokuka OlegDokuka deleted the tap-signal-listener-context-alteration-errors branch April 10, 2023 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/context This issue is related to the Context type/bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Terminate processing when SignalListener#addToContext throws
2 participants