Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature-detect "use regular streams as an underlying socket for tls.connect()" #981

Closed
dougwilson opened this issue Feb 27, 2015 · 15 comments
Labels
question Issues that look for answers. tls Issues and PRs related to the tls subsystem.

Comments

@dougwilson
Copy link
Member

Hi, I'm looking for a good solution to be able to detect if this feature exists in the runtime my module is running on so I can throw if it's not supported. It was suggested on IRC to make an issue here.

I originally thought perhaps if I just construct a duplex stream and pass it into tls.connect, but it doesn't seem to throw an error on pre-1.4.1 versions. Suggestions?

// This doesn't seem to work
var supportswrappedjsstreams = (function(){
  var s = require("stream").Duplex();s._read=s._write=function(){};
  try { require("tls").connect({ stream: s }); return true; } catch { return false; }
});
// This does work, but not sure if this is going to suddenly break in the future
var supportswrappedjsstreams = (function(){
  try { require("_stream_wrap"); return true; } catch { return false; }
});
@tellnes
Copy link
Contributor

tellnes commented Feb 27, 2015

@indutny

@chrisdickinson
Copy link
Contributor

Not sure about the state it leaves the SSL context in, but something like:

var supportswrappedjsstreams = (function(){
  var s = require("stream").Duplex(); s._read = s._write = Function();
  require('tls').connect({socket: s});
  return require('events').listenerCount('error', s) > 0;
});

should work since TLS/streamwrap will always have to install error handlers.

@dougwilson
Copy link
Member Author

Ah, yes, that does seem to work, even to weed out Node.js stuff. Thanks!

@dougwilson
Copy link
Member Author

Oops, I may have closed that too early, because I only tested the pre-1.4.1 versions; it actually seems to have 0 listeners, even on 1.4.1:

$ iojs -pe 's=require("stream").Duplex();s._read=s._write=function(){};require("tls").connect({socket:s});require("events").EventEmitter.listenerCount("error",s)'
0

$ iojs -v
v1.4.1

@dougwilson dougwilson reopened this Feb 27, 2015
@chrisdickinson
Copy link
Contributor

Ah, whoops: it should be EE.listenerCount(emitter, "eventName");. My bad!

@indutny
Copy link
Member

indutny commented Feb 27, 2015

Interesting question! Why not just check the process.version? :)

@mathiasbynens
Copy link
Contributor

Checking process.version is to user-agent sniffing as feature testing is to, uhm, feature testing. Inferring behavior from a version number seems brittle IMHO.

@indutny
Copy link
Member

indutny commented Feb 27, 2015

@mathiasbynens the proposed check is much more brittle than version checking. We have semver, so this thing can't go away until 2.0.0 for sure.

@mscdex mscdex added enhancement tls Issues and PRs related to the tls subsystem. labels Mar 12, 2015
@chrisdickinson
Copy link
Contributor

@dougwilson I think the check works now – ok to close?

@dougwilson
Copy link
Member Author

The only working check that was provided here was to sniff the version and at the same time, I'm told in this thread not to sniff the version. I need clarification still: should I start version sniffing io.js features or can someone come up with a non-version-sniffing solution to this?

@chrisdickinson
Copy link
Contributor

@dougwilson Did you see my message about the order of EE.listenerCount params?

@dougwilson
Copy link
Member Author

Oops, I missed it's specifics. Let me test that real quick. I'll either close this or provide an update today.

@dougwilson
Copy link
Member Author

@chrisdickinson I'm very sorry for the delay. This does not seem to work:

$ node -pe 's=require("stream").Duplex();s._read=s._write=function(){};require("tls").connect({socket:s});require("events").EventEmitter.listenerCount("error",s)'
2

$ node -v
v0.10.33

$ iojs -pe 's=require("stream").Duplex();s._read=s._write=function(){};require("tls").connect({socket:s});require("events").EventEmitter.listenerCount("error",s)'
1

$ iojs -v
v.1.4.1

$ iojs -pe 's=require("stream").Duplex();s._read=s._write=function(){};require("tls").connect({socket:s});require("events").EventEmitter.listenerCount("error",s)'
0

$ iojs -v
v.1.0.4

The value seems to be different in various versions and doing the > 0 seems to create false positives.

@brendanashworth brendanashworth added question Issues that look for answers. and removed enhancement labels Jun 24, 2015
@brendanashworth
Copy link
Contributor

I don't see an issue with version sniffing as long as you are conservative with major versions. Semver is for machines.

@dougwilson would you like to keep this open?

@dougwilson
Copy link
Member Author

No, we can close it, because I just ended up ignoring io.js the entire time until the merge, so never ended up taking advantage of the feature since npm never let me do version detection against io.js.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Issues that look for answers. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

No branches or pull requests

7 participants