-
Notifications
You must be signed in to change notification settings - Fork 1.9k
IGNITE-9410 Thin client transactions support #6734
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
Conversation
64654f4 to
73e1ffd
Compare
...main/java/org/apache/ignite/internal/processors/platform/client/ClientConnectionContext.java
Show resolved
Hide resolved
modules/core/src/main/java/org/apache/ignite/IgniteSystemProperties.java
Outdated
Show resolved
Hide resolved
...main/java/org/apache/ignite/internal/processors/platform/client/ClientConnectionContext.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/ignite/internal/processors/platform/client/ClientRequestHandler.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/apache/ignite/internal/processors/platform/client/tx/ClientTxContext.java
Show resolved
Hide resolved
.../src/main/java/org/apache/ignite/internal/processors/platform/client/tx/ClientTxContext.java
Outdated
Show resolved
Hide resolved
...c/main/java/org/apache/ignite/internal/processors/platform/client/tx/ClientTxEndRequest.java
Show resolved
Hide resolved
| private final ClientBinaryMarshaller marsh; | ||
|
|
||
| /** Current thread transaction. */ | ||
| private final ThreadLocal<TcpClientTransaction> tx = new ThreadLocal<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we have a possible memory leak here. TcpClientTransaction has a reference to TcpClientTransactions which has a reference to ThreadLocal<TcpClientTransaction> tx. If some TcpClientTransactions are not in use anymore but left a value in thread-local that value and TcpClientTransactions instance will never be garbage-collected. Making TcpClientTransaction class static should protect from it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't make TcpClientTransaction static, because we need TcpClientTransactions context here.
I don't quite understand how we can get possible leak here. If nobody holds the reference to TcpClientTransactions instance and nobody holds the reference to TcpClientTransaction instances, which were created by this TcpClientTransactions, the only references left outside of garbage objects it's a WeakReference to ThreadLocal from threads which have used this TcpClientTransactions instance. These weak references don't prevent to collect garbage objects.
Did I miss something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I got it. There is a weak reference only to ThreadLocal instance, but regular reference to TcpClientTransaction from the thread-local table, so memory leak is possible.
I will try to fix it tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've fixed the implementation. Also, I've found and fixed server-side transactions leak (transaction has left in active set after an attempt to end it when it's impossible to resume the transaction).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
isapego
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see several minor comments.
| import org.apache.ignite.internal.processors.platform.client.ClientResponse; | ||
|
|
||
| import java.util.LinkedHashMap; | ||
| import java.util.Map; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and in other places, imports should not be re-ordered. See this page for details: https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines#CodingGuidelines-PackageImporting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was wrong formated before this patch: Import should be ordered alphabetically, but packages starting with 'java.' should be declared prior to others.
Now it exactly matches coding guidelines.
No description provided.