search

bchavez / RethinkDb.Driver

:headphones: A NoSQL C#/.NET RethinkDB database driver with 100% ReQL API coverage.
http://rethinkdb.com/api/java
Other
383 stars 134 forks source link

ConnectionPool.Builder.ConnectAsync does not respect InitialTimeout() #143

Closed oliverjanik closed 4 years ago

oliverjanik commented 4 years ago

It seems InitialTimeout only matters to sync Connect() call.

See here: https://github.com/bchavez/RethinkDb.Driver/blob/1dd5e3d826fda0c519c5ada3df4c55eb5c6e41c3/Source/RethinkDb.Driver/Net/Clustering/ConnectionPool.cs#L549

bchavez commented 4 years ago

Yea, I think the .ConnectAsync methods, in general, ignore timeouts. Looking at Connection and SocketWrapper, the sync .Connect methods are the only ones that respect timeouts.

oliverjanik commented 4 years ago

Is it possible to change that?

If I connect using the Aync call on the fly it blocks my request forever.

I find blocking forever on await highly unusual. Most systems seems to throw TaskCancelledException or similar.

bchavez commented 4 years ago

Sure, it's possible to change it. I suppose we could support timeouts on .ConnectAsync but I also suspect the most idiomatic C# thing to do is to support CancellationToken on these async methods as well.

bchavez commented 4 years ago

Hi Oliver,

I've pushed RethinkDb.Driver v2.3.101 with the changes that resolves the ConnectAsync timeout issue. Additionally, added support for CancellationToken.

For now, I only added support for timeout and cancellation in ConnectionPool since that looked easiest. However, plumbing CancellationToken and async timeout in Connection looked a bit more involved and carried a higher impact. I'd rather test out the code changes in ConnectionPool first before modifying the more popular Connection object.

Feel free to create an issue for .ConnectAsync in Connection when you really need it.