Use a proxy based Stream decorator to support close on terminal

[moonfruit]

For #4011, here provide a redesign for Stream decorator that supports close on terminal.

Inspired by https://rmannibucau.metawerx.net/post/java-stream-decorator.

@NathanQingyangXu @vladmihalcea

[hibernate-github-bot]

Thanks for your pull request!

This pull request does not follow the contribution rules. Could you have a look?

❌ The PR title or body should list the keys of all JIRA issues mentioned in the commits
    ↳ Issue keys mentioned in commits but missing from the PR title or body: [HHH-14636]

› This message was automatically generated.

[NathanQingyangXu]

For #4011, here provide a redesign for Stream decorator that supports close on terminal.

Inspired by https://rmannibucau.metawerx.net/post/java-stream-decorator.

@NathanQingyangXu @vladmihalcea

You need to create testing case(s) to show case:

1. Without the code changes you proposed, it will fail
2. With your contribution it succeeds.

Without such proof, there is pretty slim chance to be approved or even reviewed, unless it is trivial. Seems good testing case is straightforward for this ticket. Please refer to existing testing cases when creating your own. Please try to ensure your CI building is green and solve any issue preventing it.

[moonfruit]

I think the test BasicStreamTest and JpaStreamTest is enough for this.

And without closeHandler, there is no need to add test for it should be called once on stream close.

[NathanQingyangXu]

I think the test BasicStreamTest and JpaStreamTest is enough for this.

And without closeHandler, there is no need to add test for it should be called once on stream close.

A little bit confused. It seems the existing testing cases didn't cover the bug that close would be called multiple times. If I didn't misunderstand, you are trying to solve the issue via redesigning. IMHO it would be a good testing case to expose the duplicated closing issue and you can demo that your approach makes the testing case green.

[moonfruit]

You can refer #4011.

#3834 only solved that when StreamDecorator is created, the close handler that registered by onClose() should be called once. But it ignores that the closeHandler pass to the StreamDecorator constructor will be called multi-times.

My two pull requests are focus on calling the closeHandler pass to the StreamDecorator constructor only once.

And I think that closeHandler is redundant, we should rely on Stream to take care of the close handlers. We only need to ensure that Stream.close() will be called on the terminal operations.

And I remove the field closeHandler, how can I create a test for it?

[moonfruit]

I think to solve this, the conservative way is to merge #4011 and the aggressive way is to merge this.
I can accept either of the two. But it should be fixed even not though my pull requests.

[NathanQingyangXu]

I think to solve this, the conservative way is to merge #4011 and the aggressive way is to merge this.
I can accept either of the two. But it should be fixed even not though my pull requests.

TBH, I am confused by the two PRs: which one to review? Both? That seems a little bit ambitious. As I've said, you need at least a compelling testing case in your #4011 to have it merged. I would suggest your choosing one of them as the solution. Regardless of which one to choose, ideally a compelling testing case is expected to expose the issue you are trying to solve.

[moonfruit]

You say you want an aggressive way to solve this. I provided it here. But I think the conservative way in #4011 is also enough to solve the same problems.

#4011 is safer than this but a little dirty and redundant, and not well handling the methods added in JDK above 8 like takeWhile() in 9 or mapMulti() in 16.

Either of the two can be merged. That leaves to you to leverage. Which way you like.

I can create a test to expose the issue I mentioned. But after I delete the field closeHandler to solve it I really don't know how to create a test for a non-exists field.

[NathanQingyangXu]

You can refer #4011.

#3834 only solved that when StreamDecorator is created, the close handler that registered by onClose() should be called once. But it ignores that the closeHandler pass to the StreamDecorator constructor will be called multi-times.

My two pull requests are focus on calling the closeHandler pass to the StreamDecorator constructor only once.

And I think that closeHandler is redundant, we should rely on Stream to take care of the close handlers. We only need to ensure that Stream.close() will be called on the terminal operations.

And I remove the field closeHandler, how can I create a test for it?

I think it is still testable from external behaviour which should not be bound to internal implementation detail. Not 100% sure about its feasibility but naturally without good testing case such aggressive refactoring is difficult to convince.

[moonfruit]

Please inspire me on how to do it, and I will do it.

[NathanQingyangXu]

I agree the old PR is not elegant and attractive. I think we can close it and focus on this one. Definitively we can create a good testing case for it to prove that the delegate stream is only closed once. I don't have enough bandwidth to help on this but I do think it is doable.

…

On Thu, May 27, 2021 at 10:11 PM MoonFruit ***@***.***> wrote: Please inspire me on how to do it, and I will do it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4015 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB6UYAT5DIQ7QUA5FOHY5IDTP33VJANCNFSM45T5CPDA> .

[NathanQingyangXu]

Personally I am very intrigued by this ticket. Tomorrow I might take a look at it and explore on testing possibility, but I cannot guarantee my availability. On Thu., May 27, 2021, 11:05 p.m. Nathan Xu, ***@***.***> wrote:

…

I agree the old PR is not elegant and attractive. I think we can close it and focus on this one. Definitively we can create a good testing case for it to prove that the delegate stream is only closed once. I don't have enough bandwidth to help on this but I do think it is doable. On Thu, May 27, 2021 at 10:11 PM MoonFruit ***@***.***> wrote: > Please inspire me on how to do it, and I will do it. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#4015 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AB6UYAT5DIQ7QUA5FOHY5IDTP33VJANCNFSM45T5CPDA> > . >

[moonfruit]

Add test for the basic functions of CloseOnTerminalDecorator. But still don't know how to test on a non-exists field. @NathanQingyangXu

[beikov]

I like the approach of #4011 more so I'm inclined to close this PR.

[Sanne]

Very interesting, but we should also look at how this impacts compatibility with GraalVM native images. proxies aren't easy to configure there.

[moonfruit]

@Sanne, I refactor function decorate() to support running on graalvm native images, according to DynamicProxy.

I don't know how to run tests on graalvm native images, but I test it in a standalone program, it worked.

[beikov]

@Sanne I would rather consider the following PR instead of this one: #4011
wdyt?

[moonfruit]

@beikov @Sanne What should I do to merge this?

[Sanne]

hi @moonfruit - apologies I forgot about this one.

Again - it looks interesting but I really need to check how this could work in GraalVM; since I'm not expecting to find time in 10 more days I'll see if I can find another team mate to help with that.

[moonfruit]

According to GraalVM DynamicProxy Automatic Detection, this implementation should work on GraalVM native image.

Maybe we should introduce GraalVM native image unit test like this.

[Sanne]

Yes introducing a module for GraalVM specific integration tests would really help. We even have a hibernate-graalvm already, so we could have them there.

[beikov]

The Stream API explicitly says that streams should be closed by the user if they involve IO, so IMO we should drop all decorators and just register the onClose handler on the JDK stream.