_recattrs.py: take into account exception coming from other classes

[ncoudene]

Hello and thank you for this great module !

Recently I have beginning to use loguru with an 'aiohttp' project and I have beginning to encounter some annoying exception coming from loguru :

[...]
project  |     raise ClientResponseError(
project  | aiohttp.client_exceptions.ClientResponseError: 400, message='Expected HTTP/', url=URL('https://xxxxxxxxx')
project  | 
project  | During handling of the above exception, another exception occurred:
project  | 
project  | Traceback (most recent call last):
project  |   File "/usr/local/lib/python3.11/site-packages/loguru/_handler.py", line 195, in emit
project  |     self._queue.put(str_record)
project  |   File "/usr/local/lib/python3.11/multiprocessing/queues.py", line 371, in put
project  |     obj = _ForkingPickler.dumps(obj)
project  |           ^^^^^^^^^^^^^^^^^^^^^^^^^^
project  |   File "/usr/local/lib/python3.11/multiprocessing/reduction.py", line 51, in dumps
project  |     cls(buf, protocol).dump(obj)
project  |   File "/usr/local/lib/python3.11/site-packages/loguru/_recattrs.py", line 73, in __reduce__
project  |     pickle.dumps(self.value)
project  | TypeError: can't pickle multidict._multidict.CIMultiDictProxy objects
project  | --- End of logging error ---

With the proposed patch it seems to work flawlessly again.

Let me know if it seems fine to you too.

[Delgan]

Hi @ncoudene, thanks for the improvement!

You're right that PickleError isn't the only exception that can be thrown., TypeError and a whole bunch of other errors can occur when pickle.dumps() is called. I think we should actually make try / except even more generic. This would make it possible to fix this issue: #342

Would you mind updating the PR to catch Exception instead of TypeError and PickleError please?

Also, implementing an unit test would be a nice addition. You can copy the test here but replace or extend NotPickable() so that it throws TypeError instead of PicklingError:

loguru/tests/test_add_option_enqueue.py

Lines 235 to 254 in d379fed

	def test_logging_not_picklable_exception():
	exception = None
	def sink(message):
	nonlocal exception
	exception = message.record["exception"]
	logger.add(sink, enqueue=True, catch=False)
	try:
	raise ValueError(NotPicklable())
	except Exception:
	logger.exception("Oups")
	logger.remove()
	type_, value, traceback_ = exception
	assert type_ is ValueError
	assert value is None
	assert traceback_ is None

[ncoudene]

Hi @Delgan,

I updated the PR with your requests :

• Exception is Exception instead of TypeError and PickleError
• NotPickable() throws Exception with a list of Exception and the tests have been fixed to catch it accordingly

Unfortunately the push to codecov seems to have failed.

[Delgan]

Thanks for the update, however this is not exactly what I had in mind, sorry for the confusion.

With this change, we simulate a generic Exception occurring during pickle.dumps(). The TypeError() and pickle.PicklingError() in the list are just arguments of the Exception and are not actually very relevant to the test.

I actually expected two distinct test cases:

• one with NotPicklable causing a pickle.PicklingError() (the common case) ;
• another test with NotPicklable causing a TypeError() (the improvement you suggested).

In this way, we cover different possible cases of error that can happen.

However, I understand that you might not be very familiar with the code base. I would suggest that you simply duplicate the original NotPicklable error and create a new NotPicklableTypeError class (which raises a TypeError instead of pickle.PicklingError). Then you could also duplicate test_caught_exception_queue_put() and test_not_caught_exception_queue_put() to test against this new NotPicklableTypeError() class.

Duplication is fine, I'll find a way to factorize it eventually, but we can keep it simple for now.

[Delgan]

Hi @ncoudene.

Thanks for taking the time to update the PR, I've left a comment to clarify how I think testing can be improved. If you don't have the time, let me know, I can always update your branch on my end.

Regarding the codecov error, I made an update the other day. I think if you rebase your branch against master, it should work.

[ncoudene]

Hi @Delgan ,

I revert my change and add the change you asked.

Codecov (or github actions) seems buggy.

Let me know if you have some comments.

[Delgan]

Great, thanks for taking the time to update the PR. Looks perfect. 👍

However, I think something went wrong during your rebase because some commits from the loguru/master branch were duplicated in your PR. Note that I therefore modified and force-pushed on your branch to remove them.