search

sqlectron / sqlectron-core

https://sqlectron.github.io/
MIT License
221 stars 69 forks source link

Update db/client.js #115

Closed thomaspet closed 4 years ago

thomaspet commented 4 years ago

Was this meant to be server.connecting, based on the error message?

MasterOdin commented 4 years ago

I agree this was probably meant to be server.connecting based on the error message and the fact that the immediate next check is also for database.connecting and then has an error message about the database.

However, looking more deeply at the code, I think we can just remove this check altogether as there's no reference to a connecting field in the definition of the variable: https://github.com/sqlectron/sqlectron-core/blob/2f9fdcb869038bea4f787f6a3716c0836180b65b/src/db/server.js#L14-L24

nor is there anywhere else where a server.connecting is referenced or assigned.

thomaspet commented 4 years ago

The only idea I could possibly have outside that is a magic racecondition that just happends to work with these checks :P I'll edit away the check as my pr

MasterOdin commented 4 years ago

My guess is that server.connecting might have been thought to be used in the connectTunnel logic in src/db/tunnel.js, but since it's been like that for 5 years and there's not really any issues related to needing this check, I think the theoretical race condition just doesn't happen enough in practice to care about.

Thanks for this!