tiny_tds icon indicating copy to clipboard operation
tiny_tds copied to clipboard

Separate `new` and `connect`

Open andyundso opened this issue 1 year ago • 3 comments

(this is just an idea for now)

When creating a new instance of a TinyTds::Client, it immediately tries to connect to the database. It might be a better pattern to separate the connect part to a new method that needs to be called separately.

For one, you could call escape on the Client without requiring a live connection to the server.

Second, we could investigate to hand off the GVL when calling dbopen and dbuse. Both of these functions communicate with the server. In case the connection is slow / bad, it blocks the current Ruby thread. I personally would feel more comfortable handing off the GVL within a method instead of when we initialize a new object in Ruby space.

Technically, the connect method on the client is already public. It would be more about removing it from initialize.

andyundso avatar Dec 17 '24 13:12 andyundso

For one, you could call escape on the Client without requiring a live connection to the server.

Yup, completely agree. This has always been a massive pain point for me. Another approach would be to make escape a class method, so it doesn't even need an instance to be called.

Additionally... does it even need to be in C space? This seems extremely peculiar and not nearly as robust as you'd hope/assume. What if escape was just pure Ruby?

https://github.com/rails-sqlserver/tiny_tds/blob/25ab0bdbd4092fc24230f12e6ece6bcc3caaebff/ext/tiny_tds/client.c#L488-L491

aharpervc avatar Dec 17 '24 16:12 aharpervc

I was wondering the same - I think it can be pure Ruby since it does not call anything that requires it to be in C space. And class method would also make sense. It could even be it's own separate small class (TinyTds::Encoder?)

andyundso avatar Dec 17 '24 16:12 andyundso

had another thought that might make this separation possible without requiring a breaking change: we already track in the C code if the client is closed (two times actually .. sure this has a reason but need to look into it in more details). When calling execute, we could check if the client is closed, and call the connect code automatically. This would achieve what has been discussed above: escape does not require a live connection to the server.

I am still in favor of moving escape into it's separate function and out of C. But it is a first step in the right direction.

andyundso avatar May 29 '25 12:05 andyundso