What are the issues PR 818 is trying to fix?

[Christofer Dutz <ch...@c-ware.de>]

Hi,

I’m sort of giving up on going through every change to the API in endless rounds.
I think the PR is trying to solve some issues, but more from a patchy angle, where issues are patched instead of architecturally addressing them.

So, what issues does this PR address?

From what I see, we have one issue with the connection-cache:
If something goes wrong with a connection we borrowed from the cache, then the connection might be in a broken state. As soon as the client returns the connection to the pool, it is not cleaned up. So, the next time a connection is borrowed, this broken connection is returned. I guess that’s the reason for this interceptor, that intercepts read and write operations and in case of an error returns the connection to the pool.

I think this behavior only handles the symptoms … if the client executes any operations after the failed one, it will get errors, that the connection is already returned to the pool instead of what would be the case when using a direct connection.

Instead of instantly returning the connection, I would mark the connection-lease as “broken” … and as soon as the client calls “close()” on the connection, it returns it to the pool, and invalidates the connection itself. Then we can also handle any possibly waiting clients in the queue … I think currently these would just get lost and block forever.

The other topic seems to be the timeout issue that has come up quite regularly … however here I can’t really say, if this approach fixes it … I know Lukasz is also working on something similar.

I’d really like to split up things and I guess I’ll whip up a proposal for the connection-pool changes.

Chris

Re: What are the issues PR 818 is trying to fix?

[youlin he <he...@gmail.com>]

Hi, Chris

Let me explain what is my thoughts.

If we explicitly invoke close on LeasedConnection the raw connection should
be null and return the raw connection to the pool. Then isConected is fine.
This should't throw exception. It's fine. It is logical.

If something goes wrong with a connection we borrowed from the cache, then
this orign code is inappropriate.

@Override
public boolean isConnected() {
    if(connection == null) {
        return false;
    }

    // Something goes wrong and raw connection is died.

    // On the contrary here should throw a exception let it return to the pool

    // But the raw connection still died and still in the cache.


    return connection.isConnected();

}

Christofer Dutz <ch...@c-ware.de> 于2023年2月24日周五 00:57写道：

> Hi,
>
> I’m sort of giving up on going through every change to the API in endless
> rounds.
> I think the PR is trying to solve some issues, but more from a patchy
> angle, where issues are patched instead of architecturally addressing them.
>
> So, what issues does this PR address?
>
> From what I see, we have one issue with the connection-cache:
> If something goes wrong with a connection we borrowed from the cache, then
> the connection might be in a broken state. As soon as the client returns
> the connection to the pool, it is not cleaned up. So, the next time a
> connection is borrowed, this broken connection is returned. I guess that’s
> the reason for this interceptor, that intercepts read and write operations
> and in case of an error returns the connection to the pool.
>
> I think this behavior only handles the symptoms … if the client executes
> any operations after the failed one, it will get errors, that the
> connection is already returned to the pool instead of what would be the
> case when using a direct connection.
>
> Instead of instantly returning the connection, I would mark the
> connection-lease as “broken” … and as soon as the client calls “close()” on
> the connection, it returns it to the pool, and invalidates the connection
> itself. Then we can also handle any possibly waiting clients in the queue …
> I think currently these would just get lost and block forever.
>
> The other topic seems to be the timeout issue that has come up quite
> regularly … however here I can’t really say, if this approach fixes it … I
> know Lukasz is also working on something similar.
>
> I’d really like to split up things and I guess I’ll whip up a proposal for
> the connection-pool changes.
>
> Chris
>
>