set_run_validators is not thread safe

[glyph]

Resolved: threads are bad.

Nevertheless, attr.sified objects are exactly the sorts of objects which I would use if I needed to jam some things into an inter-thread communication queue. Frozen attrs classes in particular are super handy for this purpose. So I definitely expect the library as a whole to have no global state.

Therefore, set_run_validators really ought to be using thread locals.

[Tinche]

So I'm going to go a step further and say we should deprecate and later remove the global validator switch.

My reasoning:

1. All our __init__s that contain validators pay the price of the check, even if you never use the feature.
2. It's global state.
3. I have never actually come upon any other piece of software that offers this functionality (of course - subjective) nor have I ever needed it.
4. This can be implemented on top of attrs in a number of ways. We could offer a recipe in the docs or code that replicates this functionality in a module. This code could wrap the validators explicitly or be a class decorator, but it'd be independent of our __init__ generating code.

[glyph]

I think we need something that looks like a global validator switch, to enable the use of test mocks on validated classes. Runtime type-checking is great in the default case, but having to go through 9 layers of ceremony to just stick in a None to get a quick regression test going is often not worth the hassle; folks will fall back to attr.ib() with no checking at all.

[glyph]

That said: there are perhaps better ways to implement it than having the default generated __init__ check every single time :).

[Tinche]

What if we could pull this off:

@attr.s
class A:
    a = attr.ib(validator=instance_of(int))

with disable_validators(A):
    A(None)

and remove the check in the default __init__ at the same time?

[glyph]

That would be good, with the caveat that with disable_validators(): should still work, although (as implied by the implementation strategy that I suspect you're thinking of) it might be obnoxiously slow, and make you want to just add in the specific types you want.

[Tinche]

The implementation I'm imagining would monkeypatch the __init__ on A for the duration of the context manager.

So it wouldn't work globally (unless we maintain a list of all attrs classes somewhere, and we're not going to). It also couldn't be a thread local thing, since I don't think classes can have thread local __init__s. It would allow you to create weird instances of your class for tests, and it doesn't require you to opt into this functionality beforehand.

It doesn't even need to monkeypatch, here:

@attr.s
class A:
    a = attr.ib(validator=instance_of(int))

@pytest.fixture
def my_nice_pytest_fixture():
    with disable_validators(A) as Atest:
        return Atest(None)

We create a subclass with no validators in __init__.

Anyway, I'm going to stick to my guns in claiming this is generally a misfeature, but that's neither here nor there. What bothers me much more is that not only is this feature not opt-in by default, there is no way to opt out.

[glyph]

So it wouldn't work globally (unless we maintain a list of all attrs classes somewhere, and we're not going to).

The gc module is a wonderful thing.

[glyph]

It also couldn't be a thread local thing, since I don't think classes can have thread local __init__s.

No, but they can have __init__s that check thread-local state :).

[Tinche]

The gc module is a wonderful thing.

I needed to Google the gc API before I understood this point. It's so wonderfully evil I'm on board. But how do we get Hynek to sign off on it?

No, but they can have __init__s that check thread-local state :).

This is true.

[glyph]

I think .s is proof positive that @hynek can be talked into anything. I'll see him (will I see you?) at PyCon this week.

[Tinche]

Alas, no PyCon for me. Please corner Guido in a dark alley and demand immutable dictionaries.

[MicaelJarniac]

It doesn't even need to monkeypatch, here:

@attr.s
class A:
    a = attr.ib(validator=instance_of(int))

@pytest.fixture
def my_nice_pytest_fixture():
    with disable_validators(A) as Atest:
        return Atest(None)

We create a subclass with no validators in __init__.

Anyway, I'm going to stick to my guns in claiming this is generally a misfeature, but that's neither here nor there. What bothers me much more is that not only is this feature not opt-in by default, there is no way to opt out.

How is this idea going? I just came across with attrs.validators.disabled(), but I'm on an async project, so this wouldn't be appropriate, due to its non-thread-safe nature.

My other alternative is to write my own validators with some mechanism for disabling them locally, I think.

My use case is, I wanted to have some validators for numerical fields retrieved from a database as JSON, and structured to an attrs class using cattrs. However, in some cases, I don't want it to error outright, but instead just log the bad fields and proceed with structuring the class normally.

I was about to post a new issue about this, but then noticed it is actually mentioned in the docs:

The issue I wrote but never posted.

The use of with attrs.validators.disabled(), as shown in the docs, is not async-safe, with its effects being observed outside of the scope of the context manager.

Disabling validators in a context manager in one task will effectively disable them globally, throughout all tasks, for the duration of the context manager.

This may be intended behavior, or it may be an unintended side effect. If this is not going to be fixed, I think a mention of this effect in the docs would be appropriate.

This is related to #187.

import asyncio
import attrs

@attrs.define
class Foo:
    foo: int = attrs.field(validator=attrs.validators.ge(0))

async def always_errors() -> None:
    while True:
        await asyncio.sleep(0.5)
        try:
            Foo(foo=-1)
        except ValueError:
            print("Error")
        else:
            print("No error")

async def disable_validator() -> None:
    while True:
        await asyncio.sleep(2)
        with attrs.validators.disabled():
            Foo(foo=-1)
            await asyncio.sleep(1)

async def main() -> None:
    async with asyncio.TaskGroup() as tg:
        tg.create_task(always_errors())
        tg.create_task(disable_validator())

if __name__ == "__main__":
    asyncio.run(main())

Error
Error
Error
No error
No error
Error
Error
Error
Error
No error
No error
Error
Error
Error

[hynek]

I think your best call is to move the validation into cattrs such that it's triggered when untrusted data comes in. Otherwise creating a thread-safe validator wrapper seems like the best way forward, since we're never going to add a general slowdown-for-all to make it thread-safe.

Somewhat relatedly: if you mean "async" as in "asyncio/Trio/Twisted", you don't need thread-safety if you don't await in your validators.