search

mosquito / cysystemd

systemd wrapper on Cython
Apache License 2.0
101 stars 22 forks source link

Fixed bugs in the async journal reader #29

Closed agronholm closed 4 years ago

agronholm commented 4 years ago

Fixes #28.

mosquito commented 4 years ago

Thank you for your contribution, unfortunately I worked on similar fixes last week, after my tests I released cysystemd==1.4.0 and it's conflicts with this changeset.

agronholm commented 4 years ago

No problem, so long as the bugs get fixed. I should take a look at your new code and see if it checks out.

mosquito commented 4 years ago

@agronholm actually you third case not fixed yet. I'm trying to fix it without removing AsyncReaderIterator right now.

agronholm commented 4 years ago

You can use the same approach there. I can make a PR if you like.

mosquito commented 4 years ago

@agronholm I fixed it here I will happy if you could test it on your cases

agronholm commented 4 years ago

Why did you add a .pyi file? Why not just add the type annotations to the code itself? It's not like Python < 3.5 can parse the code anyway with all those async defs and await statements there.

agronholm commented 4 years ago

Tell me what you think of my alternative fix. I'm going to bed now.

agronholm commented 4 years ago

The simplest way to iterate it would be this:

    # ... in the AsyncJournalReader class
    def __aiter__(self):
        return self

    async def __anext__(self):
        try:
            return await self._loop.run_in_executor(self._executor, self.__reader.next)
        except StopIteration:
            raise StopAsyncIteration

Do you really intend to do something about multiple concurrent iterators? Or raise an error when that is being tried?

agronholm commented 4 years ago

Waiting on a synchronous lock is admittedly not very good, but I also think that there is no need for a separate iterator class.

agronholm commented 4 years ago

Hmm – am I understanding this correctly? If I do for entry in reader: it will read entries in a non-blocking fashion until there are no entries anymore, and then we need to wait on the reader to get more entries?

mosquito commented 4 years ago

Oh... sorry I merged the wrong PR. Master has been force updated, if you still want to fix it please create a new one against master.