You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by "mrcjkb (via GitHub)" <gi...@apache.org> on 2024/07/25 09:02:05 UTC
[PR] THRIFT-4847: CancelledKeyException causes TThreadedSelectorServer to fail [thrift]
mrcjkb opened a new pull request, #3010:
URL: https://github.com/apache/thrift/pull/3010
<!-- Explain the changes in the pull request below: -->
Adds validity checks before trying to use the `SelectionKey` in `AbstractNonBlockingServer.FrameBuffer`.
<!-- We recommend you review the checklist/tips before submitting a pull request. -->
- [ ] Did you create an [Apache Jira](https://issues.apache.org/jira/projects/THRIFT/issues/) ticket? ([Request account here](https://selfserve.apache.org/jira-account.html), not required for trivial changes)
- [x] If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
- [x] Did you squash your changes to a single commit? (not required, but preferred)
- [x] Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
- [ ] If your change does not involve any code, include `[skip ci]` anywhere in the commit message to free up build resources.
<!--
The Contributing Guide at:
https://github.com/apache/thrift/blob/master/CONTRIBUTING.md
has more details and tips for committing properly.
-->
--
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: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-4847: CancelledKeyException causes TThreadedSelectorServer to fail [thrift]
Posted by "mrcjkb (via GitHub)" <gi...@apache.org>.
mrcjkb commented on PR #3010:
URL: https://github.com/apache/thrift/pull/3010#issuecomment-2268394810
Thanks for the reviews :pray: :smile:
@Jens-G is there an ETA on the next release?
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-4847: CancelledKeyException causes TThreadedSelectorServer to fail [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G commented on PR #3010:
URL: https://github.com/apache/thrift/pull/3010#issuecomment-2258070586
Reopened. So this is now a duplicate?
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-4847: CancelledKeyException causes TThreadedSelectorServer to fail [thrift]
Posted by "OneSizeFitsQuorum (via GitHub)" <gi...@apache.org>.
OneSizeFitsQuorum commented on PR #3010:
URL: https://github.com/apache/thrift/pull/3010#issuecomment-2259982330
@jeking3 Could you please review this? Many thanks!
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-4847: CancelledKeyException causes TThreadedSelectorServer to fail [thrift]
Posted by "OneSizeFitsQuorum (via GitHub)" <gi...@apache.org>.
OneSizeFitsQuorum commented on PR #3010:
URL: https://github.com/apache/thrift/pull/3010#issuecomment-2258101007
> now a duplicate?
I believe so~
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-4847: CancelledKeyException causes TThreadedSelectorServer to fail [thrift]
Posted by "mrcjkb (via GitHub)" <gi...@apache.org>.
mrcjkb commented on code in PR #3010:
URL: https://github.com/apache/thrift/pull/3010#discussion_r1691109113
##########
lib/java/src/main/java/org/apache/thrift/server/AbstractNonblockingServer.java:
##########
@@ -518,9 +522,11 @@ private boolean internalRead() {
/** We're done writing, so reset our interest ops and change state accordingly. */
private void prepareRead() {
- // we can set our interest directly without using the queue because
- // we're in the select thread.
- selectionKey_.interestOps(SelectionKey.OP_READ);
+ if (selectionKey_.isValid()) {
+ // we can set our interest directly without using the queue because
+ // we're in the select thread.
+ selectionKey_.interestOps(SelectionKey.OP_READ);
+ }
// get ready for another go-around
buffer_ = ByteBuffer.allocate(4);
state_ = FrameBufferState.READING_FRAME_SIZE;
Review Comment:
Not sure if the rest of `prepareRead` should go in the conditional block :thinking:
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-4847: CancelledKeyException causes TThreadedSelectorServer to fail [thrift]
Posted by "mrcjkb (via GitHub)" <gi...@apache.org>.
mrcjkb commented on code in PR #3010:
URL: https://github.com/apache/thrift/pull/3010#discussion_r1697093581
##########
lib/java/src/main/java/org/apache/thrift/server/AbstractNonblockingServer.java:
##########
@@ -302,6 +302,14 @@ public FrameBuffer(
}
}
+ /**
+ * Sets the selection key.
+ * @param selectionKey the new key to set.
+ */
+ public void setSelectionKey(SelectionKey selectionKey) {
Review Comment:
Not too sure about this. It feels a bit hacky and allows us to replace valid keys. Maybe someone who is familiar with the code base can suggest a better solution?
- Perhaps it would be better to add a method that clones the `FrameBuffer` with a valid key?
- What if there is an ongoing write during invalidation? Should there be some synchronisation?
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-4847: CancelledKeyException causes TThreadedSelectorServer to fail [thrift]
Posted by "mrcjkb (via GitHub)" <gi...@apache.org>.
mrcjkb commented on code in PR #3010:
URL: https://github.com/apache/thrift/pull/3010#discussion_r1697093581
##########
lib/java/src/main/java/org/apache/thrift/server/AbstractNonblockingServer.java:
##########
@@ -302,6 +302,14 @@ public FrameBuffer(
}
}
+ /**
+ * Sets the selection key.
+ * @param selectionKey the new key to set.
+ */
+ public void setSelectionKey(SelectionKey selectionKey) {
Review Comment:
Not too sure about this. It feels a bit hacky. Maybe someone who is familiar with the code base can suggest a better solution?
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-4847: CancelledKeyException causes TThreadedSelectorServer to fail [thrift]
Posted by "mrcjkb (via GitHub)" <gi...@apache.org>.
mrcjkb commented on PR #3010:
URL: https://github.com/apache/thrift/pull/3010#issuecomment-2257966486
> Relatively complete solution exists in #2171, but I don't know why this PR was Close off
Looks like it was never reviewed and closed automatically.
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-4847: CancelledKeyException causes TThreadedSelectorServer to fail [thrift]
Posted by "OneSizeFitsQuorum (via GitHub)" <gi...@apache.org>.
OneSizeFitsQuorum commented on PR #3010:
URL: https://github.com/apache/thrift/pull/3010#issuecomment-2259453870
> Actually, it might still be beneficial to add the `isValid` checks in this PR, in addition to the fix in #2171.
This is a bit like defensive programming, which is good for stability
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-4847: CancelledKeyException causes TThreadedSelectorServer to fail [thrift]
Posted by "OneSizeFitsQuorum (via GitHub)" <gi...@apache.org>.
OneSizeFitsQuorum commented on code in PR #3010:
URL: https://github.com/apache/thrift/pull/3010#discussion_r1697785441
##########
lib/java/src/main/java/org/apache/thrift/server/TThreadedSelectorServer.java:
##########
@@ -626,17 +626,23 @@ private synchronized void rebuildSelector() {
LOGGER.error("Create new Selector error.", e);
}
- for (SelectionKey key : oldSelector.selectedKeys()) {
- if (!key.isValid() && key.readyOps() == 0) continue;
+ for (SelectionKey key : oldSelector.keys()) {
+ if (!key.isValid() || key.channel().keyFor(newSelector) != null) continue;
Review Comment:
if (!key.isValid() || key.interestOps() == 0 || key.channel().keyFor(newSelector) != null)
##########
lib/java/src/main/java/org/apache/thrift/server/TThreadedSelectorServer.java:
##########
@@ -626,17 +626,23 @@ private synchronized void rebuildSelector() {
LOGGER.error("Create new Selector error.", e);
}
- for (SelectionKey key : oldSelector.selectedKeys()) {
- if (!key.isValid() && key.readyOps() == 0) continue;
+ for (SelectionKey key : oldSelector.keys()) {
+ if (!key.isValid() || key.channel().keyFor(newSelector) != null) continue;
SelectableChannel channel = key.channel();
Object attachment = key.attachment();
+ int interestOps = key.interestOps();
+ SelectionKey newKey;
try {
if (attachment == null) {
- channel.register(newSelector, key.readyOps());
+ newKey = channel.register(newSelector, interestOpts);
} else {
- channel.register(newSelector, key.readyOps(), attachment);
+ newKey = channel.register(newSelector, interestOpts, attachment);
}
+ if (attachment instanceof FrameBuffer) {
+ ((FrameBuffer) attachment.setSelectionKey(newKey);
+ }
+ key.cancel();
Review Comment:
Seems redundant as we will call oldSelector.close() later
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-4847: CancelledKeyException causes TThreadedSelectorServer to fail [thrift]
Posted by "mrcjkb (via GitHub)" <gi...@apache.org>.
mrcjkb commented on PR #3010:
URL: https://github.com/apache/thrift/pull/3010#issuecomment-2258523856
Since @zengji doesn't appear to be active anymore, I've incorporated their changes here.
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-4847: CancelledKeyException causes TThreadedSelectorServer to fail [thrift]
Posted by "mrcjkb (via GitHub)" <gi...@apache.org>.
mrcjkb commented on code in PR #3010:
URL: https://github.com/apache/thrift/pull/3010#discussion_r1698059092
##########
lib/java/src/main/java/org/apache/thrift/server/AbstractNonblockingServer.java:
##########
@@ -302,6 +302,14 @@ public FrameBuffer(
}
}
+ /**
+ * Sets the selection key.
+ * @param selectionKey the new key to set.
+ */
+ public void setSelectionKey(SelectionKey selectionKey) {
Review Comment:
My concern is that both `FrameBuffer` and `setSelectionKey` are public, so this could be a footgun in the future.
But based on your comment, I'd say YAGNI for now. I've added a note that it's not tread safe to the javadoc.
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-4847: CancelledKeyException causes TThreadedSelectorServer to fail [thrift]
Posted by "mrcjkb (via GitHub)" <gi...@apache.org>.
mrcjkb closed pull request #3010: THRIFT-4847: CancelledKeyException causes TThreadedSelectorServer to fail
URL: https://github.com/apache/thrift/pull/3010
--
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: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-4847: CancelledKeyException causes TThreadedSelectorServer to fail [thrift]
Posted by "mrcjkb (via GitHub)" <gi...@apache.org>.
mrcjkb commented on PR #3010:
URL: https://github.com/apache/thrift/pull/3010#issuecomment-2255411463
CI failure looks unrelated (?)
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-4847: CancelledKeyException causes TThreadedSelectorServer to fail [thrift]
Posted by "OneSizeFitsQuorum (via GitHub)" <gi...@apache.org>.
OneSizeFitsQuorum commented on PR #3010:
URL: https://github.com/apache/thrift/pull/3010#issuecomment-2257794458
Relatively complete solution exists in https://github.com/apache/thrift/pull/2171, but I don't know why this PR was Close off
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-4847: CancelledKeyException causes TThreadedSelectorServer to fail [thrift]
Posted by "OneSizeFitsQuorum (via GitHub)" <gi...@apache.org>.
OneSizeFitsQuorum commented on code in PR #3010:
URL: https://github.com/apache/thrift/pull/3010#discussion_r1697781111
##########
lib/java/src/main/java/org/apache/thrift/server/AbstractNonblockingServer.java:
##########
@@ -302,6 +302,14 @@ public FrameBuffer(
}
}
+ /**
+ * Sets the selection key.
+ * @param selectionKey the new key to set.
+ */
+ public void setSelectionKey(SelectionKey selectionKey) {
Review Comment:
I checked this part of the code, and in the current Reactor architecture, all the operations of the selector thread will only happen in the current thread, Inspecting the code shows that only the selector thread processes the selectionKey_ in the frameBuffer and reads and writes to transport, so there are no read or write requests in progress when rebuilding and no visibility issues (you don't need to change SelectionKey to AtomicReference when you strip it of final)
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-4847: CancelledKeyException causes TThreadedSelectorServer to fail [thrift]
Posted by "OneSizeFitsQuorum (via GitHub)" <gi...@apache.org>.
OneSizeFitsQuorum commented on code in PR #3010:
URL: https://github.com/apache/thrift/pull/3010#discussion_r1698123936
##########
lib/java/src/main/java/org/apache/thrift/server/TThreadedSelectorServer.java:
##########
@@ -626,16 +626,23 @@ private synchronized void rebuildSelector() {
LOGGER.error("Create new Selector error.", e);
}
- for (SelectionKey key : oldSelector.selectedKeys()) {
- if (!key.isValid() && key.readyOps() == 0) continue;
+ for (SelectionKey key : oldSelector.keys()) {
+ if (!key.isValid() || key.interestOps() == 0 || key.channel().keyFor(newSelector) != null) {
+ continue;
+ }
SelectableChannel channel = key.channel();
Object attachment = key.attachment();
+ int interestOps = key.interestOps();
+ SelectionKey newKey;
try {
if (attachment == null) {
- channel.register(newSelector, key.readyOps());
+ newKey = channel.register(newSelector, interestOpts);
} else {
- channel.register(newSelector, key.readyOps(), attachment);
+ newKey = channel.register(newSelector, interestOpts, attachment);
+ }
+ if (attachment instanceof FrameBuffer) {
Review Comment:
maybe we can move to line 643 as we do not need do anything when attachment is null
##########
lib/java/src/main/java/org/apache/thrift/server/AbstractNonblockingServer.java:
##########
@@ -518,9 +534,13 @@ private boolean internalRead() {
/** We're done writing, so reset our interest ops and change state accordingly. */
private void prepareRead() {
- // we can set our interest directly without using the queue because
- // we're in the select thread.
- selectionKey_.interestOps(SelectionKey.OP_READ);
+ if (selectionKey_.isValid()) {
+ // we can set our interest directly without using the queue because
Review Comment:
move these comments to line 537?
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-4847: CancelledKeyException causes TThreadedSelectorServer to fail [thrift]
Posted by "mrcjkb (via GitHub)" <gi...@apache.org>.
mrcjkb commented on PR #3010:
URL: https://github.com/apache/thrift/pull/3010#issuecomment-2258159104
Yes, this can be closed as duplicate.
Is there anything that needs to be done to get #2171 merged?
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-4847: CancelledKeyException causes TThreadedSelectorServer to fail [thrift]
Posted by "mrcjkb (via GitHub)" <gi...@apache.org>.
mrcjkb commented on PR #3010:
URL: https://github.com/apache/thrift/pull/3010#issuecomment-2258168001
Actually, it might still be beneficial to add the `isValid` checks in this PR, in addition to the fix in #2171.
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-4847: CancelledKeyException causes TThreadedSelectorServer to fail [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G merged PR #3010:
URL: https://github.com/apache/thrift/pull/3010
--
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: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org