search

scylladb / python-driver

ScyllaDB Python Driver, originally DataStax Python Driver for Apache Cassandra
https://python-driver.docs.scylladb.com
Apache License 2.0
74 stars 42 forks source link

add connect_timeout inside `set_keyspace_blocking` #259

Closed fruch closed 2 months ago

fruch commented 1 year ago

set_keyspace_blocking is called from places holding a lock, and in case that the connection is closed from the server side, it might hang forever.

using the connect_timeout on it to make sure it won't hang forever.

Ref: https://github.com/scylladb/python-driver/issues/261

fruch commented 1 year ago

@Lorak-mmk I need you review on this one, at least it's pass the integration test.

And I think it's relatively harmless change

Lorak-mmk commented 1 year ago

It would be good to have a tests that executes the timeout path - but if it's too hard to write then I think we can skip it.

fruch commented 1 year ago

It would be good to have a tests that executes the timeout path - but if it's too hard to write then I think we can skip it.

we might be able to copy the dtest casing it, but it wasn't 100% of the time...

fruch commented 1 year ago

It would be good to have a tests that executes the timeout path - but if it's too hard to write then I think we can skip it.

it might be a bit tricky, and we are still trying to find the root cause of that issue, so once fixed, that test won't be covering it anymore.

and simulated the situation which we don't know it's exact root case, it kind of futile.

Lorak-mmk commented 10 months ago

This is actually a backwards-incompatible change, the behavior of the function is different and there might be users depending on current behavior. I didn't look at the code, but wouldn't it be possible to solve this issue in wait_for_response? Intuitively, I think it should throw an exception when connection is closed.

fruch commented 10 months ago

This is actually a backwards-incompatible change, the behavior of the function is different and there might be users depending on current behavior. I didn't look at the code, but wouldn't it be possible to solve this issue in wait_for_response? Intuitively, I think it should throw an exception when connection is closed.

That's what one would expect, but some when it works with TLS behaves differently and no part identifies the connection is closed, when TLS is the lower version it doesn't hang there.

We can have higher number, but having queries with no timeout at all seems like a recipe for trouble, especially when there's a lock involved

dkropachev commented 2 months ago

Closed in favor of https://github.com/scylladb/python-driver/pull/362