You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by "FlorianHockmann (via GitHub)" <gi...@apache.org> on 2023/05/09 07:00:43 UTC
[GitHub] [tinkerpop] FlorianHockmann opened a new pull request, #2058: TINKERPOP-2944 Dispose Cancellation callbacks to fix memory leak
FlorianHockmann opened a new pull request, #2058:
URL: https://github.com/apache/tinkerpop/pull/2058
https://issues.apache.org/jira/browse/TINKERPOP-2944
This should fix the memory leak described in TINKERPOP-2944 caused by not cleaning up the cancellation token registrations which contain a reference to the `RequestMessage`.
VOTE +1
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] FlorianHockmann merged pull request #2058: TINKERPOP-2944 Dispose Cancellation callbacks to fix memory leak
Posted by "FlorianHockmann (via GitHub)" <gi...@apache.org>.
FlorianHockmann merged PR #2058:
URL: https://github.com/apache/tinkerpop/pull/2058
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] codecov-commenter commented on pull request #2058: TINKERPOP-2944 Dispose Cancellation callbacks to fix memory leak
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #2058:
URL: https://github.com/apache/tinkerpop/pull/2058#issuecomment-1539604114
## [Codecov](https://app.codecov.io/gh/apache/tinkerpop/pull/2058?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#2058](https://app.codecov.io/gh/apache/tinkerpop/pull/2058?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (27f7dbd) into [3.5-dev](https://app.codecov.io/gh/apache/tinkerpop/commit/2ee47f203ffa76b9c0c466bd8ba291010afdd831?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2ee47f2) will **increase** coverage by `0.00%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## 3.5-dev #2058 +/- ##
==========================================
Coverage 69.38% 69.39%
+ Complexity 8975 8974 -1
==========================================
Files 866 866
Lines 41251 41251
Branches 5442 5442
==========================================
+ Hits 28621 28625 +4
+ Misses 10719 10716 -3
+ Partials 1911 1910 -1
```
[see 6 files with indirect coverage changes](https://app.codecov.io/gh/apache/tinkerpop/pull/2058/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] FlorianHockmann commented on pull request #2058: TINKERPOP-2944 Dispose Cancellation callbacks to fix memory leak
Posted by "FlorianHockmann (via GitHub)" <gi...@apache.org>.
FlorianHockmann commented on PR #2058:
URL: https://github.com/apache/tinkerpop/pull/2058#issuecomment-1544154709
I made the change you suggested, @vkagamlyk. But I also had to make another change as my fix wasn't complete. It did solve the problem that the `Connection` class had an ever increasing list of cancellation registrations, but those registrations weren't disposed and were therefore still stored in the underlying `CancellationTokenSource`.
We've found that by trying out the fix in the service where we initially found the memory leak. The earlier fix here already improved the situation a lot but a memory dump revealed that the `CancellationTokenSource` had a linked list of all cancellation registrations.
The version that is now in this PR doesn't show any increasing memory usage any more (at least right now after executing the application for ~30min which was already enough earlier to see the problem, but I will update this comment tomorrow to confirm this).
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] vkagamlyk commented on pull request #2058: TINKERPOP-2944 Dispose Cancellation callbacks to fix memory leak
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on PR #2058:
URL: https://github.com/apache/tinkerpop/pull/2058#issuecomment-1544163982
LGTM
VOTE +1
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] kenhuuu commented on pull request #2058: TINKERPOP-2944 Dispose Cancellation callbacks to fix memory leak
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on PR #2058:
URL: https://github.com/apache/tinkerpop/pull/2058#issuecomment-1544900944
VOTE +1
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] vkagamlyk commented on a diff in pull request #2058: TINKERPOP-2944 Dispose Cancellation callbacks to fix memory leak
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #2058:
URL: https://github.com/apache/tinkerpop/pull/2058#discussion_r1188864810
##########
gremlin-dotnet/src/Gremlin.Net/Driver/Connection.cs:
##########
@@ -175,6 +175,7 @@ private void HandleReceivedMessage(ResponseMessage<List<object>> receivedMsg)
{
responseHandler?.Finalize(status.Attributes);
_callbackByRequestId.TryRemove(receivedMsg.RequestId.Value, out _);
+ _cancellationByRequestId.TryRemove(receivedMsg.RequestId.Value, out _);
Review Comment:
maybe it's better to move clearing of `_cancellationByRequestId` 2 lines higher to avoid leak when `responseHandler?.Finalize` failed?
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org