pnowojski commented on code in PR #27783:
URL: https://github.com/apache/flink/pull/27783#discussion_r3014673665
##########
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/channel/RecoveredChannelStateHandler.java:
##########
@@ -125,7 +147,9 @@ public void recover(
}
}
} finally {
- buffer.recycleBuffer();
+ if (!buffer.isRecycled()) {
+ buffer.recycleBuffer();
+ }
Review Comment:
But this still shows some ownership issues. `buffer` is used in this method
in two places.
```
recoverWithFiltering(channel, channelInfo, oldSubtaskIndex, buffer);
```
Do you hand over ownership of the `buffer` to `recoverWithFiltering` or not?
I think this might be not clear. If you do, and `recoverWithFiltering` is
using later this `buffer` it would be actually perfectly legal for
`recoverWithFiltering`method to immediatelly recycle this `buffer`, making the
reference invalid after `recoverWithFiltering` returns, while caller `recover`
still references this buffer later.
Later
```
channel.onRecoveredStateBuffer(buffer.retainBuffer());
```
(besides the issue that his `buffer` might be already an invalid reference)
we are copying the buffer and passing ownership of it to the
`onRecoveredStateBuffer`.
Either way, I think you should decide where you are passing ownership of the
buffer, and then owner should always be responsible for recycling it. No ifs no
buts. (we had a lot of bugs like this in the past before we cleaned up the
ownership model)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
