search

socketry / nio4r

Cross-platform asynchronous I/O primitives for scalable network clients and servers.
Other
965 stars 86 forks source link

Fix use when OpenSSL is not loaded #239

Closed MSP-Greg closed 4 years ago

MSP-Greg commented 4 years ago

Currently, errors are raised if OpenSSL is not loaded. Fixed, along with small spec change.

JFYI, I benchmarked defined? vs Object.const_defined?, and, most of the time, defined? was a little faster. Only tested on Windows mingw, 2.2 & head.

For GitHub Actions runs-on:, seems to prefer 'macos-latest', current workflow used 'macos'. Changed so CI passes on macOS MRI.

Closes https://github.com/socketry/nio4r/issues/238

ioquatix commented 4 years ago

Would it make more sense to call #to_io indiscriminately?

MSP-Greg commented 4 years ago

I haven't retested it, but my notes to the commits for it state that the reason for not using to_io in the Ruby code is to align with the c code. I said of the code before adding the SSL conditional:

When using SSL sockets, in the c extension code, monitor.io is an SSLSocket, but in Ruby code, monitor.io is a TCPSocket.

Assuming that non Windows MRI is the most prevalent use (using the c code), I thought changing the Ruby code was best. Maybe there's a better solution... IDK

ioquatix commented 4 years ago

Sorry I missed that, I just woke up so I'm still ingesting coffee to get things started.

I'm fine with this PR. #register is a reasonably hot path though.

MSP-Greg commented 4 years ago

Sorry I missed that

No problem

I just woke up so I'm still ingesting coffee

Have the same problem. I try to not write anything until I've had enough...

register is a reasonably hot path though.

I benchmarked the new conditional using both defined? & Object.const_defined?, and both were doing like 10_000_000 per second on a slow notebook, but I didn't have a lot of things loaded. I assumed it won't be an issue...