sashapolo commented on code in PR #3547:
URL: https://github.com/apache/ignite-3/pull/3547#discussion_r1555352087
##########
modules/replicator/src/main/java/org/apache/ignite/internal/replicator/ReplicaManager.java:
##########
@@ -241,19 +242,29 @@ private void onReplicaMessageReceived(NetworkMessage
message, ClusterNode sender
// If the request actually came from the network, we are already in
the correct thread that has permissions to do storage reads
// and writes.
// But if this is a local call (in the same Ignite instance), we might
still be in a thread that does not have those permissions.
- if (currentThreadCannotDoStorageReadsAndWrites()) {
+ if (shouldSwitchToRequestsExecutor()) {
requestsExecutor.execute(() -> handleReplicaRequest(request,
sender, correlationId));
} else {
handleReplicaRequest(request, sender, correlationId);
}
}
- private boolean currentThreadCannotDoStorageReadsAndWrites() {
+ private static boolean shouldSwitchToRequestsExecutor() {
if (Thread.currentThread() instanceof ThreadAttributes) {
ThreadAttributes thread = (ThreadAttributes)
Thread.currentThread();
return !thread.allows(STORAGE_READ) ||
!thread.allows(STORAGE_WRITE);
} else {
- // We don't know for sure.
+ if (PublicApiThreading.executingSyncPublicApi()) {
+ // It's a user thread, it executes a sync public API call, so
it can do anything, no switch is needed.
+ return false;
+ }
+ if (PublicApiThreading.executingAsyncPublicApi()) {
+ // It's a user thread, it executes an async public API call,
so it cannot do anything, a switch is needed.
+ return true;
+ }
+
+ // It's something else: either a JRE thread or an Ignite thread
not marked with ThreadAttributes. In any case,
Review Comment:
Should we throw an assertion here immediately?
##########
modules/workers/src/main/java/org/apache/ignite/internal/worker/ThreadAssertions.java:
##########
@@ -64,20 +59,22 @@ public static void assertThreadAllowsToRead() {
public static void assertThreadAllowsTo(ThreadOperation
requestedOperation) {
Thread currentThread = Thread.currentThread();
- if
(BLACKLISTED_THREAD_NAMES.matcher(currentThread.getName()).matches()) {
- return;
- }
-
if (!(currentThread instanceof ThreadAttributes)) {
+ if (PublicApiThreading.executingSyncPublicApi()) {
+ // Allow everything if we ride a user thread while executing a
public API call.
+
+ return;
+ }
+
LOG.warn("Thread {} does not have allowed operations",
trackerException(), currentThread);
Review Comment:
Why do we need both a warning and an exception?
##########
modules/table/src/main/java/org/apache/ignite/internal/table/AbstractTableView.java:
##########
@@ -102,13 +101,13 @@ abstract class AbstractTableView<R> implements
CriteriaQuerySource<R> {
/**
* Waits for operation completion.
*
- * @param fut Future to wait to.
+ * @param future Future to wait to.
* @param <T> Future result type.
* @return Future result.
*/
- protected final <T> T sync(Supplier<CompletableFuture<T>> fut) {
+ protected static <T> T sync(CompletableFuture<T> future) {
try {
- return fut.get().get();
+ return future.get();
Review Comment:
I think the idea was that exceptions thrown by the method that creates the
future will be handled in a similar way. Are you sure you don't break anything
with this change?
##########
modules/table/src/main/java/org/apache/ignite/internal/table/PublicApiThreadingRecordView.java:
##########
@@ -28,11 +28,13 @@
import org.jetbrains.annotations.Nullable;
/**
- * Wrapper around {@link RecordView} that adds anti-thread-hijacking
protection to methods returning {@link CompletableFuture}s.
+ * Wrapper around {@link RecordView} that maintains public API invariants
relating to threading.
+ * That is, it adds protection against thread hijacking by users and also
marks threads as 'executing a sync user operation' or
Review Comment:
Let's add a `<p>` tag here
##########
modules/core/src/main/java/org/apache/ignite/internal/lang/IgniteSystemProperties.java:
##########
@@ -70,6 +70,9 @@ public final class IgniteSystemProperties {
/** Name of the property controlling whether thread assertions are
enabled. */
public static final String THREAD_ASSERTIONS_ENABLED =
"IGNITE_THREAD_ASSERTIONS_ENABLED";
+ /** Name of the property controlling whether thread whitelisting is
enabled for thread assertions. */
+ public static final String THREAD_ASSERTIONS_THREAD_WHITELISTING_ENABLED =
"THREAD_ASSERTIONS_THREAD_WHITELISTING_ENABLED";
Review Comment:
Is this property used anywhere? I may have missed it...
--
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]
