Using with and timeout

[federicoemartinez]

Is it possible to do something like?

try:
  with create_sadlock(conn, key, timeout=10) as lock:
          # It's locked
          assert lock.locked
except TimeoutException:
          print("unable to lock")

If not, would you be interested in a PR for that?

[tanbro]

Thanks for your attention! A nice feature i think. 👍

The arguments of these lock classes' __init__ is borrowed from python stdlib's threading.Lock and multiprocessing.Lock, that's why there is no timeout.

But since timeout is so common, bring it to with statement will be nice, i think.

So a PR for that is just welcome!

[federicoemartinez]

How would you like to do it? Once could be to add a default_timeout=None to the constructor. So acquire uses that if timeout is None. The thing I see with this is that the behavior of acquire is not easy to predict when you receive an instance of Lock.

Another possibility would be to do something like

    def __call__(self, timeout=None):
        self.__timeout_for_enter = timeout
        return self

    def __enter__(self):
        success = self.acquire(timeout=self.__timeout_for_enter)
        if not success: 
                  raise Timeout()
        return self

    def __exit__(self, exc_type, exc_value, exc_tb):
        self.close()
        self.__timeout_for_enter = None

in this case, the usage pattern would be:

    sa_lock = create_sadlock(conn, key)
    with sa_lock(timeout=10) as lock:
        # It's locked
        assert lock.locked

What do you think?

[federicoemartinez]

Do you have any opinion about these options?

[tanbro]

Em... How about this [feature](https://github.com/tanbro/sqlalchemy-dlock/compare/main...feature/timeout-with-statement ?

[federicoemartinez]

I think it is ok. The value of this parameter against the one in acquire may be confusing. Maybe change it to context_manager_timeout?

[tanbro]

But, shall the parameter also be the timeout in non-context-manager case ?

[federicoemartinez]

That is a good question. I read your comments and you say:

                    When :meth:`acquire` invoked, the implementation class SHOULD keep ``timeout`` default to :data:`None`.
                    Even if we had specified a non-:data:`None` ``timeout`` previously in constructor.

I was thinking that given that we are only using that parameter as the with timeout and not as acquire default timeout, we might make that explicit using a different name for the parameter.

[federicoemartinez]

You don't like the approach of the call method, do you?

[tanbro]

ven if we had specified a non-:data:None timeout previously in constructor.

So, you may think the acquire should implicitly take the timeout if omitted.

But i think the timeout is only for the with clause, and shall not be applied to a normal acquire.

[federicoemartinez]

I think it is ok to have the timeout from init applying only to "with". In that case I suggest to use a different name in the constructor, something like timeout_for_with or maybe a better name. I think that having to parameters timeout (I mean with the same name) but different meanings can be confusing.

[tanbro]

I think it is ok to have the timeout from init applying only to "with". In that case I suggest to use a different name in the constructor, something like timeout_for_with or maybe a better name. I think that having to parameters timeout (I mean with the same name) but different meanings can be confusing.

👍 agree

[federicoemartinez]

When do you think you can have this merged?

[tanbro]

The commit 50b44bde1e3b4256bd5e11ff9c0985ff2c2907d3 solved the issue.

Now, the arguemnt's name is contextual_timeout