fix: treat external connection as open #1243
Conversation
nechaido
changed the title
|
It looks like it is now the same check as in line 64 Line 64 in 6f17518 And so this might make sense 🤔 However I'm now not sure anymore how the code behaves when it comes to Lines 64 to 70 in 6f17518 Do we want to check for Beside that, you need to run the formatter to make lint happy 😸 |
So ATM the connection status of the external is ignored and looking at the code it looks like the external connection is assumed to be a valid initialized connection. So the current implementation seems like it is in the spirit of the original. If you feel like it I can replace the check on Line 64 in 6f17518 .connected() invocation.
in the meanwhile, i have added a test for this behaviour. |
I also thought about this. So we could add an internal function like const connected: DBConnection['connected'] = () => {
return isExternalClient || connectionStatus === ConnectionStatus.CONNECTED
}and then use this in both places. One more further thinking idea that came into my mind is: Should we add a |
|
@Shinigami92 I have both extracted a check and introduced a new connection status. |
|
I will merge and release this when I have dedicated time and my non-company PC Do you think this is a bugfix (-> |
This is a bug fix for me as I encountered it after starting to scale my API servers horizontally. So I'd vote on P.S.: it would be cool to implement a If you are interested in this -- I can submit the pr sometimes later. |
Shinigami92
changed the title
Sounds good to me |
This addresses: #885
Here is the resulting "check trace" on why advisory lock is never released for external Client
exhibit A: It checks that the connection is connected and only if it is connected -- releases the lock
exhibit B: Connection status is decided by this variable
exhibit C: When the client is created the connection status is not updated
exhibit D: The initial value is "DISCONNECTED"
Alternatively, this can be fixed by:
connectionStatusvariable toConnectionStatus.CONNECTEDThe provded solution seems more like the most one.
If any maintainer can advise on a better solution -- I would be glad to update the PR.