Fixed bug: Skip closing of io socket if it's already closed.

[thorsteneckel]

Hi there!

We're currently experimenting with TCR for mocking LDAP connections. However, I encountered an issue where a SystemStackError is raised while closing the (SSL) connection. What basically happens is: close LDAP connection -> Close SSL connection -> Close TCR mocked socket + close IO socket -> Close TCR mocked socket ->Close SSL connection -> ....

This can be easily avoided by checking if the IO socket is already closed before performing a close on it. This is what this PR does.

Let me know if there are any questions.

[HarlemSquirrel]

I know this has been open for a while but it would be awesome to have a test for it.

[HarlemSquirrel]

@thorsteneckel ☝️

[thorsteneckel]

Hey @HarlemSquirrel - thanks for taking the time reviewing and reviving this PR. I had a look how and where to add a test in the codebase. I couldn't find any related tests. The change that introduced the FixSSLSocketSyncClose class was done about 6 years ago and doesn't come with any tests.

However, there is TestSSLBER which "tests" some things around the SSL Socket (OpenSSL::SSL::SSLSocket) which is the socket we're taking about in this context: https://github.com/ruby-ldap/ruby-net-ldap/blob/master/test/test_ssl_ber.rb#L23-L29

In the scenario of this PR the error actually (probably?) only occurs in the context of mocking LDAP connections with TCR. Which is quite handy. On the other hand, TCR is far from wide adoption. I rather would not introduce it as a test dependency.

What do you think? Should we introduce TCR as a testing dependency?
I'd guess checking a socket for the closed state before actually closing it wouldn't hurt. It's what Ruby does under the hood anyway – but in the context of TCR it's later/too late.

How would you approach/expect a test?

Looking forward to your reply.

[HarlemSquirrel]

@thorsteneckel thanks for the details here. If we're unable to reproduce this with "real" connections then I'm hesitant to make it part of the library since I feel it is generally a bad idea to add complexity only to facilitate testing. Perhaps it would be better to modify TCR to behave more like that Ruby code you shared.
If there's some other benefits we can identify then I'd be happy to make this part of this library.

[thorsteneckel]

@HarlemSquirrel thanks for your fast and reasonable reply. I understand your concerns and agree – switching the angle to a maintainers and owners perspective. We'll try to figure out how to address this in the scope of TCR. Closing therefore.