gremlin-go: fix deadlock on (*DriverRemoteConnection).Close#1754
Conversation
|
@simonz-bq @lyndonbauto you might want to give this one a look because you already have the context of #1734. Thanks! |
Further analysis shows that apache#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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 3.5-dev #1754 +/- ##
===========================================
+ Coverage 63.48% 63.58% +0.09%
===========================================
Files 23 23
Lines 3640 3636 -4
===========================================
+ Hits 2311 2312 +1
+ Misses 1150 1145 -5
Partials 179 179 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Edit: nvm overall lgtm +1 Minor suggestion in the changes |
| err := connection.protocol.close() | ||
| if err != nil { | ||
|
|
||
| // This callback is called from within protocol.readLoop. Therefore, |
There was a problem hiding this 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().
There was a problem hiding this comment.
Sure thing. Done in c93661f. I did the same in transporter.writeLoop.
4 participants
Further analysis shows that #1734 is not the right fix. A deadlock can still happen in
(*gremlinServerWSProtocol).readLoopiferrorCallbackis called before the connection protocol is closed and, because of this,(*gremlinServerWSProtocol).closecalls(*gremlinServerWSProtocol).wg.Wait.This commit adds the parameter "wait" to
protocol.close. So, the call to(*gremlinServerWSProtocol).wg.Waitcan be skipped when called from within an error callback. Also, the test that was introduced has been updated.