You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by GitBox <gi...@apache.org> on 2022/05/26 01:10:21 UTC
[GitHub] [tinkerpop] simonz-bq opened a new pull request, #1669: Fix deadlock on non response-specific read errors
simonz-bq opened a new pull request, #1669:
URL: https://github.com/apache/tinkerpop/pull/1669
- Fixed a potential deadlock that would occur whenever reading from the server fails in a fashion that is not specific to a specific request, e.g. the connection is dead
- Fixed a potential edge case where a ResultSet would not return an error (it would only log it) alongside an incomplete set of results
--
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 #1669: Fix deadlock on non response-specific read errors
Posted by GitBox <gi...@apache.org>.
vkagamlyk commented on code in PR #1669:
URL: https://github.com/apache/tinkerpop/pull/1669#discussion_r882929461
##########
gremlin-go/driver/resultSet.go:
##########
@@ -120,6 +121,19 @@ func (channelResultSet *channelResultSet) Close() {
}
}
+// Close and remove from the channelResultSet from the container without locking container. Meant for use when calling
+// function already locks the container.
+func (channelResultSet *channelResultSet) unlockedClose() {
Review Comment:
what happens if 2 go routines calls this method simultaneously?
--
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 #1669: Fix deadlock on non response-specific read errors
Posted by GitBox <gi...@apache.org>.
vkagamlyk commented on code in PR #1669:
URL: https://github.com/apache/tinkerpop/pull/1669#discussion_r882929461
##########
gremlin-go/driver/resultSet.go:
##########
@@ -120,6 +121,19 @@ func (channelResultSet *channelResultSet) Close() {
}
}
+// Close and remove from the channelResultSet from the container without locking container. Meant for use when calling
+// function already locks the container.
+func (channelResultSet *channelResultSet) unlockedClose() {
Review Comment:
what happens if 2 go routines this method simultaneously?
--
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 #1669: Fix deadlock on non response-specific read errors
Posted by GitBox <gi...@apache.org>.
lyndonbauto commented on PR #1669:
URL: https://github.com/apache/tinkerpop/pull/1669#issuecomment-1139801653
VOTE+1
Because this is a minor bug fix and I have full context on this issue I am going to my vote alone.
--
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] simonz-bq commented on a diff in pull request #1669: Fix deadlock on non response-specific read errors
Posted by GitBox <gi...@apache.org>.
simonz-bq commented on code in PR #1669:
URL: https://github.com/apache/tinkerpop/pull/1669#discussion_r882905041
##########
gremlin-go/driver/resultSet.go:
##########
@@ -120,6 +121,19 @@ func (channelResultSet *channelResultSet) Close() {
}
}
+// Close and remove from the channelResultSet from the container without locking container. Meant for use when calling
Review Comment:
Close is still used in the happy path where a ResultSet is fully read, as well as being an exposed point for the user to mark it as completed. This makes it hard for us to modify the API around that.
--
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 merged pull request #1669: Fix deadlock on non response-specific read errors
Posted by GitBox <gi...@apache.org>.
lyndonbauto merged PR #1669:
URL: https://github.com/apache/tinkerpop/pull/1669
--
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 a diff in pull request #1669: Fix deadlock on non response-specific read errors
Posted by GitBox <gi...@apache.org>.
lyndonbauto commented on code in PR #1669:
URL: https://github.com/apache/tinkerpop/pull/1669#discussion_r882894831
##########
gremlin-go/driver/resultSet.go:
##########
@@ -120,6 +121,19 @@ func (channelResultSet *channelResultSet) Close() {
}
}
+// Close and remove from the channelResultSet from the container without locking container. Meant for use when calling
Review Comment:
If it is used, perhaps we should have
`LockedClose()`
and
`unlockedClose()`
and both should call
```
func (channelResultSet *channelResultSet) close(locked bool) {
if !channelResultSet.closed {
channelResultSet.channelMutex.Lock()
channelResultSet.closed = true
if lock {
channelResultSet.container.delete(channelResultSet.requestID)
} else {
delete(channelResultSet.container.internalMap, channelResultSet.requestID)
}
close(channelResultSet.channel)
channelResultSet.channelMutex.Unlock()
channelResultSet.sendSignal()
}
}```
--
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 a diff in pull request #1669: Fix deadlock on non response-specific read errors
Posted by GitBox <gi...@apache.org>.
lyndonbauto commented on code in PR #1669:
URL: https://github.com/apache/tinkerpop/pull/1669#discussion_r883782304
##########
gremlin-go/driver/resultSet.go:
##########
@@ -120,6 +121,19 @@ func (channelResultSet *channelResultSet) Close() {
}
}
+// Close and remove from the channelResultSet from the container without locking container. Meant for use when calling
+// function already locks the container.
+func (channelResultSet *channelResultSet) unlockedClose() {
Review Comment:
I think the idea is that this should never be called outside of closeAll and closeAll uses a mutex to ensure there is no concurrent usage.
--
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 #1669: Fix deadlock on non response-specific read errors
Posted by GitBox <gi...@apache.org>.
vkagamlyk commented on code in PR #1669:
URL: https://github.com/apache/tinkerpop/pull/1669#discussion_r883788180
##########
gremlin-go/driver/resultSet.go:
##########
@@ -120,6 +121,19 @@ func (channelResultSet *channelResultSet) Close() {
}
}
+// Close and remove from the channelResultSet from the container without locking container. Meant for use when calling
+// function already locks the container.
+func (channelResultSet *channelResultSet) unlockedClose() {
Review Comment:
it makes sense
--
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 a diff in pull request #1669: Fix deadlock on non response-specific read errors
Posted by GitBox <gi...@apache.org>.
lyndonbauto commented on code in PR #1669:
URL: https://github.com/apache/tinkerpop/pull/1669#discussion_r882892698
##########
gremlin-go/driver/resultSet.go:
##########
@@ -120,6 +121,19 @@ func (channelResultSet *channelResultSet) Close() {
}
}
+// Close and remove from the channelResultSet from the container without locking container. Meant for use when calling
Review Comment:
Is `Close` still used? Overall this LGTM and I think solves this well - just wondering if we can remove the other Close() now.
--
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 #1669: Fix deadlock on non response-specific read errors
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #1669:
URL: https://github.com/apache/tinkerpop/pull/1669#issuecomment-1138042757
# [Codecov](https://codecov.io/gh/apache/tinkerpop/pull/1669?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 [#1669](https://codecov.io/gh/apache/tinkerpop/pull/1669?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8ec7a7c) into [3.5-dev](https://codecov.io/gh/apache/tinkerpop/commit/f59c47cab9d3e7e385bee2634ecbce92dec074f9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f59c47c) will **increase** coverage by `0.30%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## 3.5-dev #1669 +/- ##
===========================================
+ Coverage 62.96% 63.27% +0.30%
===========================================
Files 23 23
Lines 3416 3553 +137
===========================================
+ Hits 2151 2248 +97
- Misses 1111 1131 +20
- Partials 154 174 +20
```
| [Impacted Files](https://codecov.io/gh/apache/tinkerpop/pull/1669?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [gremlin-go/driver/connection.go](https://codecov.io/gh/apache/tinkerpop/pull/1669/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-Z3JlbWxpbi1nby9kcml2ZXIvY29ubmVjdGlvbi5nbw==) | `80.00% <0.00%> (-1.16%)` | :arrow_down: |
| [gremlin-go/driver/protocol.go](https://codecov.io/gh/apache/tinkerpop/pull/1669/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-Z3JlbWxpbi1nby9kcml2ZXIvcHJvdG9jb2wuZ28=) | `63.20% <0.00%> (+0.99%)` | :arrow_up: |
| [gremlin-go/driver/resultSet.go](https://codecov.io/gh/apache/tinkerpop/pull/1669/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-Z3JlbWxpbi1nby9kcml2ZXIvcmVzdWx0U2V0Lmdv) | `80.18% <0.00%> (-7.45%)` | :arrow_down: |
| [gremlin-go/driver/strategies.go](https://codecov.io/gh/apache/tinkerpop/pull/1669/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-Z3JlbWxpbi1nby9kcml2ZXIvc3RyYXRlZ2llcy5nbw==) | `65.69% <0.00%> (-1.48%)` | :arrow_down: |
| [gremlin-go/driver/traversal.go](https://codecov.io/gh/apache/tinkerpop/pull/1669/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-Z3JlbWxpbi1nby9kcml2ZXIvdHJhdmVyc2FsLmdv) | `70.54% <0.00%> (ø)` | |
| [gremlin-go/driver/graphTraversal.go](https://codecov.io/gh/apache/tinkerpop/pull/1669/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-Z3JlbWxpbi1nby9kcml2ZXIvZ3JhcGhUcmF2ZXJzYWwuZ28=) | `89.11% <0.00%> (+0.77%)` | :arrow_up: |
| [gremlin-go/driver/serializer.go](https://codecov.io/gh/apache/tinkerpop/pull/1669/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-Z3JlbWxpbi1nby9kcml2ZXIvc2VyaWFsaXplci5nbw==) | `64.76% <0.00%> (+1.39%)` | :arrow_up: |
| [gremlin-go/driver/graphBinary.go](https://codecov.io/gh/apache/tinkerpop/pull/1669/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-Z3JlbWxpbi1nby9kcml2ZXIvZ3JhcGhCaW5hcnkuZ28=) | `53.79% <0.00%> (+2.75%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/tinkerpop/pull/1669?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/tinkerpop/pull/1669?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [f59c47c...8ec7a7c](https://codecov.io/gh/apache/tinkerpop/pull/1669?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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