Fixes/workaround for deadlocks (and follow-on issues) observed in exception cases

[mpontillo]

This branch is a partial fix for the following issue:
czerwonk/atlas_exporter#12

For the ripeatlas portion of this, we are attempting to work around the following issues:

• Calling c.Close() within the OnDisconnection handler is redundant, and leads to a recursive call and a deadlock.
• Calling close(ch) upon receiving an atlas_error is redundant because the channel will be closed within the OnDisconnection handler as well.
• It is possible, given the current overall design of the code, that a write to a closed channel could be attempted. Use recover() to prevent a crash. (A larger effort would be needed to solve this in a "more correct" way.)

[glightfoot]

One of the possible solutions to the hacky recover solution would be to add identical functions that take a channel as an argument, instead of creating and managing the channel in the ripeatlas package. This allows for a persistent channel that is does not have to be closed, but it does add some additional management responsibilities to the user. Happy to throw together a draft PR once this is merged to demonstrate if that's something that sounds amenable.

[glightfoot]

@jelu Here is the PR that we made to our fork with another solution that avoids calling recover() by avoiding the channel lifecycle issues. See the description and PR for the use and examples of each digitalocean#3

[jelu]

@glightfoot I'm a bit reluctant to add that much code and new functions to something that looks very simple, just close the connection on the relative actions and only close the channel on the OnDisconnection event as this diff. This should solve the problem without the use of recover().

[glightfoot]

@jelu that's what we tried at first, but eventually there is a panic on send to a closed channel as there is no synchronous way to guarantee the inloop is shut down when the close is called. The only way to guarantee that nothing ever writes to the channel after closing it would be to either protect the writes to the channel with a mutex to check if isAlive has been set to false and close the channel there, but that adds more contention.

The recover hack was the minimal change we got to work running for a long time with no panics, and adding the WithChan funcs was how we got around the hack. Unfortunately, I don't see a good way to avoid the hacks and more code without modifying the golang-socketio library.

[jelu]

I've just added a control channel that should solve any problem with "send on closed channel", see my branch fix.

[glightfoot]

That's certainly safer, although there's still a chance of a panic on send since the operations are concurrent and not atomic. If we really want to avoid the recover, there should at least be a sleep of a couple seconds between closing the ctrl chan and the results chan to effectively ensure that case won't happen in the real world

[jelu]

Ended up wrapping it all in a Mutex.

@glightfoot @mpontillo I suggest that you create your own streamer for your specific needs.

[mpontillo]

Thanks for your consideration @jelu, and for your contributions to the community.
We may end up maintaining the digitalocean/ripeatlas fork in order to pursue more aggressive changes. Of course, in accordance with the LGPL, all of our work related to this project will remain public.

[jelu]

@mpontillo Maybe I need to explain more what I mean here, you do not need to maintain your own fork.
You can simply just make your own streamer which depends on this library.
And if you want to be atlaser interface compatible, simply create new func's on the side to set/get the control channel for the closing event and other stuff as needed, and don't modify MeasurementLatest or add new calls just for additional parameters.

[mpontillo]

What we may need to do, eventually, is write some test cases to demonstrate the need for some of the more extensive changes we are proposing. To do that, I think, would require significant enough refactoring that it would not be appropriate for us to ask you to merge those changes. We'll keep you updated on how we approach this. Thanks for your feedback.

[jelu]

@mpontillo Sounds good and test/use cases is indeed very good to have. Looking forward to see what this might be :)