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/07/12 02:04:07 UTC
[GitHub] [tinkerpop] jroimartin opened a new pull request, #1754: gremlin-go: fix deadlock on (*DriverRemoteConnection).Close
jroimartin opened a new pull request, #1754:
URL: https://github.com/apache/tinkerpop/pull/1754
Further analysis shows that apache/tinkerpop#1734 is not the right fix. A deadlock can still happen in `(*gremlinServerWSProtocol).readLoop` if `errorCallback` is called before the connection protocol is closed and, because of this, `(*gremlinServerWSProtocol).close` calls `(*gremlinServerWSProtocol).wg.Wait`.
This commit adds the parameter "wait" to `protocol.close`. So, the call to `(*gremlinServerWSProtocol).wg.Wait` can be skipped when called from within an error callback. Also, the test that was introduced has been updated.
Tests may fail while #1753 is not merged.
--
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 pull request #1754: gremlin-go: fix deadlock on (*DriverRemoteConnection).Close
Posted by GitBox <gi...@apache.org>.
simonz-bq commented on PR #1754:
URL: https://github.com/apache/tinkerpop/pull/1754#issuecomment-1183620876
@jroimartin, correct me if I'm wrong, but another deadlock can occur here. Let's call them user for invoked `close` and readLoop for error-caused `close`.
1. `readLoop` errors and calls the `errorCallback`, which in turn calls `close`
2. `readLoop` holds the `close` lock
3. `readLoop` finishes closing, releases the `close` lock, goes back to the loop, and invokes `Done()` before returning
5. user holds the `close` lock
6. user waits for the waitgroup
There is a race condition here where we assume step 6 happens before step 3 finishes.
I *think* this can be solved by saving the `closed` state before to a variable, and then checking that && wait to solve 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] simonz-bq commented on a diff in pull request #1754: gremlin-go: fix deadlock on (*DriverRemoteConnection).Close
Posted by GitBox <gi...@apache.org>.
simonz-bq commented on code in PR #1754:
URL: https://github.com/apache/tinkerpop/pull/1754#discussion_r920462382
##########
gremlin-go/driver/connection.go:
##########
@@ -55,8 +55,10 @@ type connectionSettings struct {
func (connection *connection) errorCallback() {
connection.logHandler.log(Error, errorCallback)
connection.state = closedDueToError
- err := connection.protocol.close()
- if err != nil {
+
+ // This callback is called from within protocol.readLoop. Therefore,
Review Comment:
Can we modify protocol.readLoop?
Simply I would just like all `protocol.wg.Done()` to be removed, and instead added before the `for` as `defer(protocol.wg.Done()`.
--
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 merged pull request #1754: gremlin-go: fix deadlock on (*DriverRemoteConnection).Close
Posted by GitBox <gi...@apache.org>.
spmallette merged PR #1754:
URL: https://github.com/apache/tinkerpop/pull/1754
--
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] jroimartin commented on pull request #1754: gremlin-go: fix deadlock on (*DriverRemoteConnection).Close
Posted by GitBox <gi...@apache.org>.
jroimartin commented on PR #1754:
URL: https://github.com/apache/tinkerpop/pull/1754#issuecomment-1181239879
@simonz-bq @lyndonbauto you might want to give this one a look because you already have the context of #1734. 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] jroimartin commented on a diff in pull request #1754: gremlin-go: fix deadlock on (*DriverRemoteConnection).Close
Posted by GitBox <gi...@apache.org>.
jroimartin commented on code in PR #1754:
URL: https://github.com/apache/tinkerpop/pull/1754#discussion_r921110961
##########
gremlin-go/driver/connection.go:
##########
@@ -55,8 +55,10 @@ type connectionSettings struct {
func (connection *connection) errorCallback() {
connection.logHandler.log(Error, errorCallback)
connection.state = closedDueToError
- err := connection.protocol.close()
- if err != nil {
+
+ // This callback is called from within protocol.readLoop. Therefore,
Review Comment:
Sure thing. Done in c93661f. I did the same in `transporter.writeLoop`.
--
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 #1754: gremlin-go: fix deadlock on (*DriverRemoteConnection).Close
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #1754:
URL: https://github.com/apache/tinkerpop/pull/1754#issuecomment-1181662350
# [Codecov](https://codecov.io/gh/apache/tinkerpop/pull/1754?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 [#1754](https://codecov.io/gh/apache/tinkerpop/pull/1754?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e92d53f) into [3.5-dev](https://codecov.io/gh/apache/tinkerpop/commit/92560d97d0a39642d62c6aa195d67458c5f49438?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (92560d9) will **decrease** coverage by `0.03%`.
> The diff coverage is `71.42%`.
```diff
@@ Coverage Diff @@
## 3.5-dev #1754 +/- ##
===========================================
- Coverage 63.48% 63.45% -0.04%
===========================================
Files 23 23
Lines 3640 3642 +2
===========================================
Hits 2311 2311
- Misses 1150 1152 +2
Partials 179 179
```
| [Impacted Files](https://codecov.io/gh/apache/tinkerpop/pull/1754?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/1754/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==) | `77.77% <20.00%> (-2.23%)` | :arrow_down: |
| [gremlin-go/driver/protocol.go](https://codecov.io/gh/apache/tinkerpop/pull/1754/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=) | `64.61% <100.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/tinkerpop/pull/1754?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/1754?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 [92560d9...e92d53f](https://codecov.io/gh/apache/tinkerpop/pull/1754?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