ThreadedHistory: invoke history load callback on event loop

[bobhy]

Successor to #1159, but with a nice, compact commit history. Here is the original description:

Fixes issue #1158, and seems to work in Xonsh as well.

Depends on ThreadedHistory and Application being able to rendezvous on same event loop. While this may not be possible for history in general, I think it is an acceptable restriction on ThreadedHistory.

[jonathanslenders]

Thanks for this! I'm looking into it. The test script is very helpful.

I won't merge it yet because I'm looking into two things, which aren't obvious:

• This breaks the loading of the history in ptpython (and I guess IPython as well), where we swap out event loops for different event loops, after the history class has been created.
• I want it to be possible for the history loading to be implemented as an async generator (some kind of async stream to which buffers can subscribe). The threaded loading should be build on top of that. It's a bit complex, because the loading of the history entries happens on exactly the opposite side of this stream from the new entries that are added by the application user.

I would also like to make sure that multiple Buffer instances can share the same History instance. I believe this has not been done so far, but the code was somewhat written to make this possible.

It's pretty complex to get the interaction between the history and the Buffer class completely correct and flexible enough for all use cases. I don't want to rush into it and get this right before merging it. I hope to have some time for it myself in the coming days/weeks.

[bobhy]

Hi Jonathan, Could you point me at code samples where ptpython and ipython swap the event loop? There's nothing magic about the event loop in this PR, perhaps I can fix it to accommodate those scenarios. Regards, Bob

…

On Sat, Aug 1, 2020, 2:10 PM Jonathan Slenders ***@***.***> wrote: Thanks for this! I'm looking into it. The test script is very helpful. I won't merge it yet because I'm looking into two things, which aren't obvious: - This breaks the loading of the history in ptpython (and I guess IPython as well), where we swap out event loops for different event loops, after the history class has been created. - I want it to be possible for the history loading to be implemented as an async generator (some kind of async stream to which buffers can subscribe). The threaded loading should be build on top of that. It's a bit complex, because the loading of the history entries happens on exactly the opposite side of this stream from the new entries that are added by the application user. I would also like to make sure that multiple Buffer instances can share the same History instance. I believe this has not been done so far, but the code was somewhat written to make this possible. It's pretty complex to get the interaction between the history and the Buffer class completely correct and flexible enough for all use cases. I don't want to rush into it and get this right before merging it. I hope to have some time for it myself in the coming days/weeks. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1170 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAPBIZBW6YBCXVSJ6RQKMPDR6SADZANCNFSM4ODRE4CA> .

[jonathanslenders]

See: https://github.com/prompt-toolkit/ptpython/blob/master/ptpython/repl.py#L70
Thanks!

[jaraco]

Installing this branch into my xonsh environment, I ran into a new error:

~ $ cd ~/p/pypException in thread Thread-10:
Traceback (most recent call last):
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/threading.py", line 932, in _bootstrap_inner
    self.run()
  File "/Users/jaraco/.local/pipx/venvs/xonsh/lib/python3.8/site-packages/xonsh/history/__amalgam__.py", line 372, in run
    files = self.files(only_unlocked=True)
  File "/Users/jaraco/.local/pipx/venvs/xonsh/lib/python3.8/site-packages/xonsh/history/__amalgam__.py", line 426, in files
    if lj["locked"] and lj["ts"][0] < boot:
  File "/Users/jaraco/.local/pipx/venvs/xonsh/lib/python3.8/site-packages/xonsh/__amalgam__.py", line 783, in __getitem__
    rtn = self._getitem_mapping(key)
  File "/Users/jaraco/.local/pipx/venvs/xonsh/lib/python3.8/site-packages/xonsh/__amalgam__.py", line 767, in _getitem_mapping
    offset = self.offsets[key]
KeyError: 'locked'

Perhaps it's unrelated, because I also updated xonsh at the same time.

[bqv]

I've been using this patch for two weeks, it's solved the error and hasn't created any new ones, for me

[bobhy]

@jonathanslenders Please review now.
Added an event_loop argument to ThreadedHistory so caller can provide whatever event_loop is desired. Modified slow-history.py example to demo it. New argument is not mandatory and defaults to asyncio.get_event_loop(), but raises DeprecationWarning if caller doesn't specify. Given that ThreadedHistory must be instantiated before the prompt, I feel developer really should specify the event loop (and Application and friends should also allow this!). You could convince me to remove the DeprecationWarning, however.

[bqv]

Why has this not been merged yet?

@bobhy perhaps it's an idea to just fork this and upload a new package to pypi, since jonathan seems to have been given up maintaining this repo for the last month

[bqv]

@jonathanslenders.

[bqv]

@jonathanslenders?

[jonathanslenders]

I haven't given up. Just other priorities. Open source projects are not dead if there are a few months of inactivity.

This PR is breaking ptpython, because the prompt runs in a different event loop. Actually with recent changes, the prompt even runs in a different thread. This is because we don't want the prompt to share the same loop with potential asyncio code running in the background. There are many challenges related to concurrency. See: https://github.com/prompt-toolkit/ptpython/blob/9900c8bb98262cffe9685c2fb9a7b7d07c2039dc/docs/concurrency-challenges.rst Most of it is working however.

The problem is that the whole application can be created in one thread. (Like the Buffer, ThreadedHistory objects, etc...), but eventually, when the application starts, it will run in a different thread. There is no guarantee that the original event loop from the first thread will still be running, and the thread in which the application runs can be created long after the application was created. So the event loop is not yet known when the ThreadedHistory object is created. This is how ptpython operates right now, and it's something I don't like to change. To make it even more complex, every .run()/.prompt() call in ptpython will run in a fresh thread in the latest code.

A few other things:

• I'd like to have the History implementation similar to what we have for the Completer classes, where the low-level APIs are async, and we build threading on top of that.
• I don't like the current inheritance that we have in History right now.
• I prefer to not merge code with comments like "ugly reference to base class internal" and TODOs. This still needs to be fixed. I very much appreciate the contribution, but there's still a bit more work to be done.

Probably get_strings should become an asynchronous generator, and ThreadedHistory should populate an async stream that can be consumed multiple times in an async way from different threads. (Like itertools.tee, but then for an async generator.)

I hope to get some time for this in the coming days.

[jonathanslenders]

Hi all, I have an improved implementation ready that I'm about to merge: #1328

Special thanks to @bobhy for the original history improvement proposal and script to reproduce the Xonsh bug. That has been proven very useful. In the end, I could not merge this PR, because it would break ptpython and possibly other tools, but I'm pretty satisfied with what I have right now.

Thanks again to all for the patience!

[bobhy]

I can take a look at enabling the event loop swap out. I see how the legacy code does it now. Are you thinking about exposing an API for PTK to do this in a standard way? I could sketch one out as part of this...

[jonathanslenders]

Hi @bobhy, thanks, but I think we're good now. I merged #1328 which seems to fix the issue, and has the requirements for the history API I had in mind. (to support async, like with the completions.) I did test with your test script from this PR, I tested with Xonsh, ptpython and IPython and all still seem to work well. So I went ahead and pushed prompt_toolkit 3.0.11 to PyPI.

If you have some time, it would be great if you can test your Xonsh setup with the latest prompt_toolkit from PyPI now.

[jonathanslenders]

Fixed in prompt_toolkit 3.0.14.