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

Fix NullPointerException in FuseableStreamMessage #4859

Merged
merged 2 commits into from May 8, 2023

Conversation

ikhoon
Copy link
Contributor

@ikhoon ikhoon commented May 5, 2023

Related issue: #4852

Motivation:

FuseableStreamMessage.collect() throws a NPE when only mapError() is applied to a normal StreamMessage.

StreamMessage.of(1, 2, 3, 4)
             .mapError(IllegalStateException::new)
             // Raises a NullPointerException
             .collect().join();

The function field in FuseableStreamMessage can be null if no map() is applied to FuseableStreamMessage.

The problem only happens when collect() method is called. subscribe() method already takes the case into account.

Modifications:

  • Use the original object as is if function is null.

Result:

Related issue: line#4852

Motivation:

`FuseableStreamMessage.collect()` throws a NPE when only `mapError()` is
applied to a normal `StreamMessage`.
```
StreamMessage.of(1, 2, 3, 4)
             .mapError(IllegalStateException::new)
             // Raises a NullPointerException
             .collect().join();
```

`function` field in `FuseableStreamMessage` can be null if no `map()` is
applied to `FuseableStreamMessage`.
https://github.com/line/armeria/blob/871d87297e4d051241589cb1ae95641cbc83f880/core/src/main/java/com/linecorp/armeria/common/stream/FuseableStreamMessage.java#L162

The problem only happens when `collect()` method is called.
`subscribe()` method already takes the case into account.

Modifications:

- Use the original object as is if `function` is null.

Result:

- You no longer see `NullPointerException` when `.mapError()` is applied
  to `StreamMessage`
- Closes line#4852
@ikhoon ikhoon added the defect label May 5, 2023
@ikhoon ikhoon added this to the 1.24.0 milestone May 5, 2023
@Lincong
Copy link

Lincong commented May 6, 2023

Thanks @ikhoon for the quick fix (which LGTM)!

Before we upgrade our Armeria dependency to pick up this fix, is adding a no-op .mapData(d => d) a reasonable and sustainable workaround?

@ikhoon
Copy link
Contributor Author

ikhoon commented May 7, 2023

Right. Adding .mapData(d => a) is a correct workaround to avoid the problem.

Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix @ikhoon 👍 🙇 🚀

Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

Thanks! 👍

Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

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

Thanks @Lincong for the detailed report and @ikhoon for a quick fix! 🙇

@trustin trustin merged commit 92e7017 into line:main May 8, 2023
10 of 11 checks passed
@ikhoon ikhoon deleted the mapError-npe branch May 25, 2023 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NPE on HttpResponse callback In gRPC client
5 participants