vzlogger
vzlogger copied to clipboard
volkszaehler
reading_thread: Only allow cancellation at defined points
This is needed to properly support pthread_cancel(), which causes the metermap test to fail every so often with the error "terminate called without an active exception"
https://blog.memzero.de/pthread-cancel-noexcept/ has a good explanation on why this works.
in general a good topic! I'm not convinced yet on the implementation. E.g. the long lasting tasks are not cancelled but some fast/short loops are.
The read in (prev line 78) e.g. could be blocking for a long time (e.g. while waiting for new data...
Unfortunately the read thread is implemented in "Meter", so specific cancellation points would have to be implemented in the different Meter implementations. Doing this in the main loop and as part of the sleep is not the best solution, but it is a start.
My main motivation was to make sure the test does not crash anymore, which is what this patch satisfies. An even better solution would be to use C++ threads, and to create cancel request mutexes, but that would be a larger refactoring.
thx! see comment / question regarding usage of deferred cancelation type. In general I'd prefer:
- change to default deferred cancellation
- check code / loops that might not be able to handle deferred cancellation and disable cancellation only around those code parts (didn't even check whether that's the current default?)
All C++ destructiors are non-cancellable (which is a generic issue when you mix c++ and pthreads). So anything needs to be non-cancellable unless specifically marked.
@mbehr1: is this good to merge by now? sadly this is a bit beyond my C++ skills (and time available to learn). it seens scary that this had been missed since the switch to C++.
i'm wondering if the better solution wouldn't be to stop using pthread_cancel() altogether,
quoting from https://blog.memzero.de/pthread-cancel-noexcept/ :+1:
In general pthread_cancel should not be used in c++ code at all, but the thread should have a way to request a clean shutdown
i guess the only advantage of pthread_cancel is that it can interrupt system calls like read() or sleep() that might occur in a meter's meter_read() code
@r00t- I think the code is ok/safe. But it might have the side effect that terminating vzlogger might take a while (depending on the meter and how long the read blocks). (while could even be kind of really long if e.g. the meter gets very few inputs/events)
Because this pull request is removing a blocker (failing tests) for several merges I would suggest merging it and track the remaining TODOs in a separate ticket.
after all, this should not have been merged without more testing, see the user report in #571
should not have been merged without more testing
There was a reason to merge this. In my opinion we should be grateful a user did the work of testing it. We should offer releases we consider reasonably stable while encouraging users to use HEAD.