Fix freeze in hdl_grabber.cpp

[SunlayGGX]

Close the udp socket before stopping HDLGrabber. Fix only for Windows.
Linked to issue #4460

[mvieth]

@SunlayGGX Hi, thanks for this PR. It would be great if we could fix this for all systems, not only windows. I looked into the Boost documentation, which says that receive_from will complete with the boost::asio::error::operation_aborted error. So could this work:
if (e.code () != boost::asio::error::operation_aborted) ?

I am also wondering: shouldn't the part after the catch block (if (isAddressUnspecified ...) be inside the try block? If receive_from threw the exception, that part shouldn't be executed, right?

Unfortunately, I don't have a velodyne device for proper testing ...

[SunlayGGX]

@mvieth Hi, thanks for reviewing my PR.

You're right about using boost error code, I'll change that. Though it's not boost::asio::error::operation_aborted but boost::asio::error::interrupted that is triggered, at least on Windows case.
I don't know if it will be right on Linux or MacOS and since I don't have those setup, I cannot test them, hence my Windows only fix.
Should I remove the ifdef and ignore boost::asio::error::interrupted and boost::asio::error::operation_aborted errors ?

For the second part of your review, let's do it like you proposed because the result is the same (enqueueHDLPacket is protected by a check if length is of the right size).

Finally, you don't need a Velodyne to test. As said in the issue #4460, before the fix : the freeze happens when you cannot connect to one (either it isn't seen on the network, or it doesn't exist) and try to close the grabber.
I'm confident the fix didn't change the behavior when the Velodyne successfully connects, it was tested on my side on Windows.

[mvieth]

@SunlayGGX I tested with Ubuntu, no Velodyne device connected, and only calling hdl_read_socket_->close (); does not seem to be sufficient because the receive_from call is not interrupted. If I add hdl_read_socket_->shutdown(boost::asio::ip::tcp::socket::shutdown_both); before calling close () , like the Boost documentation suggests, then the receive_from call is interrupted, but shutdown throws an exception saying Transport endpoint is not connected. With the shutdown call, enclosed in a try-catch, it works on Ubuntu. If that works on Windows as well, we could add that before calling close()

Should I remove the ifdef and ignore boost::asio::error::interrupted and boost::asio::error::operation_aborted errors ?

Yes please, that sounds good

[mvieth]

Thank you, now the freeze is fixed on Linux, too 👍

[SunlayGGX]

Thank you so much for your review @mvieth .
I'll leave the merge of that PR to you if you're authorized, or anybody that could do it whenever they want.

[mvieth]

Thank you so much for your review @mvieth . I'll leave the merge of that PR to you if you're authorized, or anybody that could do it whenever they want.

I wanted to give @larshg the opportunity to take a look before I merge it 🙂

[larshg]

Sorry about the delay 😄