search

adjust / rmq

Message queue system written in Go and backed by Redis
MIT License
1.57k stars 206 forks source link

Should we care about closing the connection #60

Closed silasdavis closed 5 years ago

silasdavis commented 5 years ago

I note from: https://github.com/adjust/rmq/issues/40 that redisConnection is deliberately not exported, but the Connection interface does not have the Close() method. There is therefore no way outside of local declaration scope to close a connection. Should I care?

silasdavis commented 5 years ago

Just came across Cleaner, so I guess that can be used to get to the closed state. Would be useful to have some guidance on how and why this is meant to be used.

wellle commented 5 years ago

@silasdavis: Why would you want to close a connection? What should happen to the open queues, consumers and producers in that case?

I agree the cleaner is not properly documented, it's mentioned in the https://github.com/adjust/rmq#todo section though.

silasdavis commented 5 years ago

Why would you want to close a connection?

I dunno, superstition...? Mostly just because it was part of the public API and I was wondering if Bad Things would happen. Seems not.

I have the cleaner running and run it on startup so I assume that will save me.

What should happen to the open queues, consumers and producers in that case?

Perhaps they shutdown gracefully?

I was more just worried about any resources that might need to be cleaned up. Happy to close if there is no need to call Close

wellle commented 5 years ago

It's part of the public interface because the cleaner uses it. (I know it's the same package so technically it wouldn't have to, but that was the motivation behind exporting it)

But I'm not aware of any use case where you'd have to close a connection. You can safely ignore that function.

silasdavis commented 5 years ago

Thanks!