Cannot unpickle DocumentNode

[corydolphin]

Reporting issues with GraphQL-core 3

Unpickling of DocumentNode(s) fail since unpickling a FrozenList fails. I believe the root issue is that List is extended, and pickle assumes it may call append, resulting in a FrozenError.

[Cito]

Hm. This is not nice.

Actually I think it was a mistake to use a FrozenList runtime type for these attributes. It would have been better to use FrozenList as a protocol only. I had so far shied away from using protocols because they were introduced in Py 3.8 only. But I think we can import typing_extensions for older Python version.

Will try to work on it this week.

[corydolphin]

Thanks for the quick response! If the goal is only to have a protocol, we can use sequence (which is immutable) and a Mapping (also immutable) types, which have the added benefit of being covariant. I expect these types were not intentionally part of the public API, but arguably leaked. I can make the types as deprecated but retain them in case anyone was invoking them directly, or remove them. Up to you. I’m happy to make that change if you’d be able to help on the administrative side to get it released :).

…

On Tue, Oct 20, 2020 at 11:42 AM Christoph Zwerschke < ***@***.***> wrote: Hm. This is not nice. Actually I think it was a mistake to use a FrozenList runtime type for these attributes. It would have been better to use FrozenList as a protocol only. I had so far shied away from using protocols because they were introduced in Py 3.8 only. But I think we can import typing_extensions for older Python version. Well try to work on it this week. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#112 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADNDKJ6KTRJY27G5K54YXLSLXDXJANCNFSM4SXRLWHA> .

[Cito]

Good point. You're right, Mapping is immutable already. So Sequence and Mapping look like the proper types to use where I currently use FrozenList and FrozenDict.

I think we should show a deprecation warning when somebody imports the FrozenList or FrozenDict modules telling them to use List/Sequence or Dict/Mapping instead, and then eventually remove these modules.

If you like, you can send a PR otherwise I will work on this. Make sure the tox tests are passing before submittting.

[Cito]

@corydolphin I like to move this forward, but I have limited time, so I would be glad if you or anybody else could help.

The first thing we need is a test that makes sure that document asts can be properly pickled and unpickled. So I added this at the bottom of test_schema_parser.py:

    def can_pickle_and_unpickle_kitchen_sink_schema(kitchen_sink_sdl):  # noqa: F811
        sys.setrecursionlimit(2000)
        doc = parse(kitchen_sink_sdl)
        dumped = pickle.dumps(doc)
        loaded = pickle.loads(dumped)
        assert loaded == doc

This test fails due to the FrozenError issue and passes if we replace the FrozenLists with an ordinary list.

However, it baffles me that I had to set sys.setrecursionlimit(2000) to make it pass, otherweise I get a RecursionError. The kitchen_sink document is actually not that complicated or deeply nested. Does anybody understand what's going on here?

[Cito]

However, it baffles me that I had to set sys.setrecursionlimit(2000) to make it pass, otherweise I get a RecursionError. The kitchen_sink document is actually not that complicated or deeply nested. Does anybody understand what's going on here?

I have investigated this now. The reason is that the AST nodes keep a reference to their locations which keep a reference to their tokens which form a double-linked-list. Pickling this list is possible, but very expensive. The simplest solution is to modify the Token class so that the links are not pickled. They are only needed while parsing or for debugging purposes, not for using a schema.

[Cito]

This has now been solved in b9423b7 by using tuples instead of FrozenLists for lists of nodes, and by not pickling the Token links, as explained above. Will be available in v3.2.0rc4.