fix: catch all exceptions in forked process #921
Conversation
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
jjaakola-aiven
force-pushed
the
jjaakola-aiven-catch-all-exceptions-in-forked-process
branch
3 times, most recently
from
b00cc77 to
56c4f9e
Compare
| # todo: This will leave residues on sys.path in case of exceptions. If really must | ||
| # mutate sys.path, we should at least wrap in try-finally. |
There was a problem hiding this comment.
cannot we do as final action a tree delete marking the protobuf folder as the father folder to delete? In that way (unless the path of the dependencies contain ../../name we can delete everything eventually)
There was a problem hiding this comment.
I know you didn't introduce the comment but can be solved in that way
There was a problem hiding this comment.
I think the protobuf files are ok to keep, if the one exists it will be reused and protoc is not called.
The mangling of sys.path and the lines below should be wrapped in try-except-else-finally block to remove the added path if something fails. But not removed if it already was there.
karapace/protobuf/io.py
Outdated
| @@ -233,34 +237,37 @@ def __init__( | |||
| def read(self, bio: BytesIO) -> dict: | |||
| if self._reader_schema is None: | |||
| self._reader_schema = self._writer_schema | |||
| return reader_mp(self.config, self._writer_schema, self._reader_schema, bio) | |||
| return read_in_forked_process_multiprocess_process(self.config, self._writer_schema, self._reader_schema, bio) | |||
There was a problem hiding this comment.
I could remove the extra _process_ though.
| @@ -168,25 +168,25 @@ def read_data( | |||
| return class_instance | |||
|
|
|||
|
|
|||
| _ReaderQueue: TypeAlias = "Queue[dict[object, object] | Exception]" | |||
| _ReaderQueue: TypeAlias = "Queue[dict[object, object] | BaseException]" | |||
karapace/protobuf/io.py
Outdated
| @@ -233,34 +237,37 @@ def __init__( | |||
| def read(self, bio: BytesIO) -> dict: | |||
| if self._reader_schema is None: | |||
| self._reader_schema = self._writer_schema | |||
| return reader_mp(self.config, self._writer_schema, self._reader_schema, bio) | |||
| return read_in_forked_process_multiprocess_process(self.config, self._writer_schema, self._reader_schema, bio) | |||
| @@ -274,14 +281,18 @@ def writer_mp( | |||
| # To avoid problem with enum values for basic SerDe support we | |||
| # will isolate work with call protobuf libraries in child process. | |||
| if __name__ == "karapace.protobuf.io": | |||
| queue: _WriterQueue = Queue() | |||
| p = Process(target=writer_process, args=(queue, config, writer_schema, message_name, datum)) | |||
| writer_queue: _WriterQueue = Queue(1) | |||
There was a problem hiding this comment.
why only 1 message in the queue?
There was a problem hiding this comment.
does it make sense to proceed with just 1 at a time?
There was a problem hiding this comment.
There is nothing else put to queue than one message at a time which the caller waits for. I decided just to make the size explicit here.
There was a problem hiding this comment.
didn't notice wasn't reused and we have 1 process for each message
jjaakola-aiven
force-pushed
the
jjaakola-aiven-catch-all-exceptions-in-forked-process
branch
from
56c4f9e to
c945648
Compare
If base exception is raised the communication queue will block in the calling process and will cause timeouts. This is prevalent in unit test for protobuf serialization in Github Actions, caused by the protobuf compiler work directory existence. The fix is to use pathlib.Path with exists=True and parents=True for removing the FileExistsError and also changing the error handling logic to pass back also BaseExceptions through the multiprocess queue.
eliax1996
force-pushed
the
jjaakola-aiven-catch-all-exceptions-in-forked-process
branch
from
c945648 to
8f0a350
Compare
If base exception is raised the communication queue will block in the calling process and will cause timeouts. This is prevalent in unit test for protobuf serialization in Github Actions, caused by the protobuf compiler work directory existence. The fix is to use pathlib.Path with exists=True and parents=True for removing the FileExistsError and also changing the error handling logic to pass back also BaseExceptions through the multiprocess queue.
About this change - What it does
References: #xxxxx
Why this way