You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by "ministat (via GitHub)" <gi...@apache.org> on 2023/02/05 15:05:12 UTC
[GitHub] [tinkerpop] ministat opened a new pull request, #1908: TINKERPOP-2837 Fix NPE caused by a thread safe issue
ministat opened a new pull request, #1908:
URL: https://github.com/apache/tinkerpop/pull/1908
The current annotations object created by Collections.synchronizedMap() will be shared across multiple threads, and some methods which access this object do not apply lock (synchronized), as a result, NPE occurs. This patch fixed this issue.
--
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] spmallette commented on pull request #1908: TINKERPOP-2837 Fix NPE caused by a thread safe issue
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on PR #1908:
URL: https://github.com/apache/tinkerpop/pull/1908#issuecomment-1425880289
looking at this further reveals that annotations will lose their order. i assume providers inserting annotations would want to preserve the order they insert things as it may make a difference in presentation to the user. note that a test fails in 3.6-dev that isn't here on 3.5-dev as a result of this change. i think we'd want to find a fix that doesn't alter this behavior somehow.
--
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] ministat commented on pull request #1908: TINKERPOP-2837 Fix NPE caused by a thread safe issue
Posted by "ministat (via GitHub)" <gi...@apache.org>.
ministat commented on PR #1908:
URL: https://github.com/apache/tinkerpop/pull/1908#issuecomment-1486508777
@xiazcy I cannot reproduce the issue through UT. This only occurs in my production environment. So, If a UT is a must, I do not want to continue invest on 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] codecov-commenter commented on pull request #1908: TINKERPOP-2837 Fix NPE caused by a thread safe issue
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #1908:
URL: https://github.com/apache/tinkerpop/pull/1908#issuecomment-1355024445
# [Codecov](https://codecov.io/gh/apache/tinkerpop/pull/1908?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 [#1908](https://codecov.io/gh/apache/tinkerpop/pull/1908?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e140eea) into [3.5-dev](https://codecov.io/gh/apache/tinkerpop/commit/341793e9da19b1474dd2c39982907cb2fbac574d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (341793e) will **decrease** coverage by `5.59%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## 3.5-dev #1908 +/- ##
=============================================
- Coverage 69.32% 63.72% -5.60%
=============================================
Files 865 24 -841
Lines 41037 3664 -37373
Branches 5407 0 -5407
=============================================
- Hits 28450 2335 -26115
+ Misses 10667 1149 -9518
+ Partials 1920 180 -1740
```
| [Impacted Files](https://codecov.io/gh/apache/tinkerpop/pull/1908?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...emlin/process/traversal/util/ImmutableMetrics.java](https://codecov.io/gh/apache/tinkerpop/pull/1908/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9wcm9jZXNzL3RyYXZlcnNhbC91dGlsL0ltbXV0YWJsZU1ldHJpY3MuamF2YQ==) | | |
| [...gremlin/driver/handler/WebSocketClientHandler.java](https://codecov.io/gh/apache/tinkerpop/pull/1908/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1kcml2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3RpbmtlcnBvcC9ncmVtbGluL2RyaXZlci9oYW5kbGVyL1dlYlNvY2tldENsaWVudEhhbmRsZXIuamF2YQ==) | | |
| [...mlin/process/traversal/step/filter/FilterStep.java](https://codecov.io/gh/apache/tinkerpop/pull/1908/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9wcm9jZXNzL3RyYXZlcnNhbC9zdGVwL2ZpbHRlci9GaWx0ZXJTdGVwLmphdmE=) | | |
| [...mlin/structure/util/empty/EmptyVertexProperty.java](https://codecov.io/gh/apache/tinkerpop/pull/1908/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9zdHJ1Y3R1cmUvdXRpbC9lbXB0eS9FbXB0eVZlcnRleFByb3BlcnR5LmphdmE=) | | |
| [.../strategy/optimization/MatchPredicateStrategy.java](https://codecov.io/gh/apache/tinkerpop/pull/1908/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9wcm9jZXNzL3RyYXZlcnNhbC9zdHJhdGVneS9vcHRpbWl6YXRpb24vTWF0Y2hQcmVkaWNhdGVTdHJhdGVneS5qYXZh) | | |
| [.../gremlin/process/traversal/step/PathProcessor.java](https://codecov.io/gh/apache/tinkerpop/pull/1908/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9wcm9jZXNzL3RyYXZlcnNhbC9zdGVwL1BhdGhQcm9jZXNzb3IuamF2YQ==) | | |
| [...pop/gremlin/driver/ser/SerializationException.java](https://codecov.io/gh/apache/tinkerpop/pull/1908/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1kcml2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3RpbmtlcnBvcC9ncmVtbGluL2RyaXZlci9zZXIvU2VyaWFsaXphdGlvbkV4Y2VwdGlvbi5qYXZh) | | |
| [...emlin/language/grammar/TraversalMethodVisitor.java](https://codecov.io/gh/apache/tinkerpop/pull/1908/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9sYW5ndWFnZS9ncmFtbWFyL1RyYXZlcnNhbE1ldGhvZFZpc2l0b3IuamF2YQ==) | | |
| [.../apache/tinkerpop/gremlin/structure/io/Mapper.java](https://codecov.io/gh/apache/tinkerpop/pull/1908/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9zdHJ1Y3R1cmUvaW8vTWFwcGVyLmphdmE=) | | |
| [...emlin/process/traversal/step/map/ConstantStep.java](https://codecov.io/gh/apache/tinkerpop/pull/1908/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9wcm9jZXNzL3RyYXZlcnNhbC9zdGVwL21hcC9Db25zdGFudFN0ZXAuamF2YQ==) | | |
| ... and [831 more](https://codecov.io/gh/apache/tinkerpop/pull/1908/diff?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] lyndonbauto commented on pull request #1908: TINKERPOP-2837 Fix NPE caused by a thread safe issue
Posted by GitBox <gi...@apache.org>.
lyndonbauto commented on PR #1908:
URL: https://github.com/apache/tinkerpop/pull/1908#issuecomment-1358056914
@ministat Change LGTM, however is it possible to create a test case that breaks before your change and works after?
--
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] xiazcy commented on pull request #1908: TINKERPOP-2837 Fix NPE caused by a thread safe issue
Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy commented on PR #1908:
URL: https://github.com/apache/tinkerpop/pull/1908#issuecomment-1483323009
Hi @ministat, do you have any plans to come back to this one? 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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on pull request #1908: TINKERPOP-2837 Fix NPE caused by a thread safe issue
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on PR #1908:
URL: https://github.com/apache/tinkerpop/pull/1908#issuecomment-1490127461
thanks for looking into this further. unfortunately, it's creating more questions. Not sure how others feel but I'm hesitant to include another dependency here to solve this, especially a deprecated one that is not maintained anymore. it's also changing gryo which would make server backward incompatible with drivers, e.g. 3.5.6 server would stop working with a 3.5.5 driver. gryo is deprecated i suppose but i'd say it remains a concern in my mind.
Given those issues, I'd like this approach to be a last resort and even then I'd consider relegating it to a newer version that doesn't break compatibility. Do you feel like we have reached a position of last resort at this point? If so, could you share other approaches you've tried that didn't really work well?
thanks for your patience on this - sometimes the smallest bugs can be a nuisance to fix and generate a lot of discussion.
--
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] ministat commented on pull request #1908: TINKERPOP-2837 Fix NPE caused by a thread safe issue
Posted by "ministat (via GitHub)" <gi...@apache.org>.
ministat commented on PR #1908:
URL: https://github.com/apache/tinkerpop/pull/1908#issuecomment-1416684777
The UT cannot reproduce this issue. Only the production env can see it. If the UT is a must, I suggest abondoning this PR.
--
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] spmallette commented on pull request #1908: TINKERPOP-2837 Fix NPE caused by a thread safe issue
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on PR #1908:
URL: https://github.com/apache/tinkerpop/pull/1908#issuecomment-1416721813
sorry - lost track of this one. i don't know that a unit test was a must. lyndon just asked for one. i'm curious about performance implications. anyway, i don' think you needed to close it if you were having a problem.
--
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] ministat commented on pull request #1908: TINKERPOP-2837 Fix NPE caused by a thread safe issue
Posted by GitBox <gi...@apache.org>.
ministat commented on PR #1908:
URL: https://github.com/apache/tinkerpop/pull/1908#issuecomment-1359134322
It is not easy to reproduce this thread safe issue in a UT, anyway let me try.
--
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] ministat commented on pull request #1908: TINKERPOP-2837 Fix NPE caused by a thread safe issue
Posted by "ministat (via GitHub)" <gi...@apache.org>.
ministat commented on PR #1908:
URL: https://github.com/apache/tinkerpop/pull/1908#issuecomment-1418014601
@spmallette What performance implications do you mean here? Do you mean the performance comparison of Collections.synchronizedMap and ConcurrentHashMap? If yes, https://www.baeldung.com/java-synchronizedmap-vs-concurrenthashmap already gives the result.
--
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] ministat commented on pull request #1908: TINKERPOP-2837 Fix NPE caused by a thread safe issue
Posted by "ministat (via GitHub)" <gi...@apache.org>.
ministat commented on PR #1908:
URL: https://github.com/apache/tinkerpop/pull/1908#issuecomment-1488086097
@spmallette To address your concern, I think a concurrentlinkedhashmap is required. Maybe this one: https://github.com/ben-manes/concurrentlinkedhashmap
--
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] ministat closed pull request #1908: TINKERPOP-2837 Fix NPE caused by a thread safe issue
Posted by "ministat (via GitHub)" <gi...@apache.org>.
ministat closed pull request #1908: TINKERPOP-2837 Fix NPE caused by a thread safe issue
URL: https://github.com/apache/tinkerpop/pull/1908
--
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] spmallette commented on pull request #1908: TINKERPOP-2837 Fix NPE caused by a thread safe issue
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on PR #1908:
URL: https://github.com/apache/tinkerpop/pull/1908#issuecomment-1486627203
at this point i think the issue is less with unit tests and more with my comment here:
https://github.com/apache/tinkerpop/pull/1908#issuecomment-1425880289
--
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] ministat commented on pull request #1908: TINKERPOP-2837 Fix NPE caused by a thread safe issue
Posted by "ministat (via GitHub)" <gi...@apache.org>.
ministat commented on PR #1908:
URL: https://github.com/apache/tinkerpop/pull/1908#issuecomment-1491517303
@spmallette Thanks for your attention on this. Well, for this issue, I did not find other approaches. I have temporarily applied ConcurrentHashMap in my local environment even though it sacrisfied the insertion order as you pointed. I agreed to archive this issue before we have a good 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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org