Skip to content
Browse files

[refactor] Refactor for http.Agent APIs

  • Loading branch information...
1 parent aebfec9 commit db0cce03852648375c75c7d7ea10442e7ea05378 @indexzero committed May 23, 2011
Showing with 143 additions and 104 deletions.
  1. +143 −104 lib/websocket.js
View
247 lib/websocket.js
@@ -1,8 +1,41 @@
+/*
+ * Copyright (c) 2010, Peter Griess <pg@std.in>
+ * https://github.com/pgriess/node-websocket-client
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ *
+ * * Redistributions of source code must retain the above copyright notice,
+ * this list of conditions and the following disclaimer.
+ *
+ * * Redistributions in binary form must reproduce the above copyright notice,
+ * this list of conditions and the following disclaimer in the documentation
+ * and/or other materials provided with the distribution.
+ *
+ * * Neither the name of node-websocket-client nor the names of its
+ * contributors may be used to endorse or promote products derived from this
+ * software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
+ * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+ * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE
+ * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
+ * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+ * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+ * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ */
+
var assert = require('assert');
var buffer = require('buffer');
var crypto = require('crypto');
var events = require('events');
var http = require('http');
+var https = require('https');
var net = require('net');
var urllib = require('url');
var sys = require('sys');
@@ -146,16 +179,6 @@ var str2hex = function(str) {
return out.trim();
};
-// Get the scheme for a URL, undefined if none is found
-var getUrlScheme = function(url) {
- var i = url.indexOf(':');
- if (i == -1) {
- return undefined;
- }
-
- return url.substring(0, i);
-};
-
// Set a constant on the given object
var setConstant = function(obj, name, value) {
Object.defineProperty(obj, name, {
@@ -469,120 +492,136 @@ var WebSocket = function(url, proto, opts) {
// that http.Client passes its constructor arguments through,
// un-inspected to net.Stream.connect(). The latter accepts a
// string as its first argument to connect to a UNIX socket.
- var httpClient = undefined;
- switch (getUrlScheme(url)) {
- case 'ws':
- var u = urllib.parse(url);
- httpClient = http.createClient(u.port || 80, u.hostname);
+ var protocol, agent, port, u = urllib.parse(url);
@pgriess
pgriess added a note May 24, 2011

One per line, please.

@indexzero
Owner
indexzero added a note May 25, 2011

Sure. I'll keep that in mind in the future. Do you want me to change this or can you update it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ if (u.protocol === 'ws:' || u.protocol === 'wss:') {
+ protocol = u.protocol === 'ws:' ? http : https;
@pgriess
pgriess added a note May 24, 2011

Can you put parens around the conditional part of the ternary expressions you're using (i.e. here and below)? That's the style used elsewhere in this file (except for the line which you are removing here which was busted, heh).

@indexzero
Owner
indexzero added a note May 25, 2011

I didn't notice the style you're pointing out. Are you suggesting: protocol = u.protocol === 'ws:' ? (http) : https; ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ port = u.protocol === 'ws:' ? 80 : 443;
@pgriess
pgriess added a note May 24, 2011

Looks like the getAgent() code picks correct defaults for you already.

@indexzero
Owner
indexzero added a note May 25, 2011

Yes. Can kill this line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ agent = u.protocol === 'ws:' ? protocol.getAgent(u.hostname, u.port || port) : protocol.getAgent({
@pgriess
pgriess added a note May 24, 2011

Does Agent work with HTTP upgrade? I can't tell. It seems to support the event, but I can't tell if it is good about not re-using an upgraded connection, etc.

In this case, I don't think it's all that useful for us to re-use the cached Agent since we're 1:1 WS:TCP connection. The premise of Agents is transparent keep-alive/TCP re-use, AFAICT. Directly constructing an Agent also normalizes the construction syntax, since we build them both with an options dictionary.

@indexzero
Owner
indexzero added a note May 25, 2011

This is a question better suited for @ry, maybe he can shed some light on how Agent instances work with upgraded HTTP. The agent does emit the upgrade event, but I'm not sure how it remaps future connections onto connections that are already upgraded.

I've actually been thinking about doing away with calls to .getAgent() in node-http-proxy as well, so no objection here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ host: u.hostname,
+ port: u.port || port
+ });
+
httpPath = (u.pathname || '/') + (u.search || '');
httpHeaders.Host = u.hostname + (u.port ? (":" + u.port) : "");
- break;
-
- case 'ws+unix':
- var sockPath = url.substring('ws+unix://'.length, url.length);
- httpClient = http.createClient(sockPath);
- httpHeaders.Host = 'localhost';
- break;
-
- default:
- throw new Error('Invalid URL scheme \'' + urlScheme + '\' specified.');
}
+ else if (u.protocol === 'ws+unix') {
@pgriess
pgriess added a note May 24, 2011

On second thought, how hard is it to retain this?

The URL parser seems to break on schemes with a plus character. Was there some other reason that you nuked this?

@indexzero
Owner
indexzero added a note May 25, 2011

Not hard at all. I really just assumed that the URL parser would work on this. Perhaps the best overall is to patch core, maybe @isaacs can come in to the rescue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ throw new Error('ws+unix is not implemented');
+ // var sockPath = url.substring('ws+unix://'.length, url.length);
+ // httpClient = http.createClient(sockPath);
+ // httpHeaders.Host = 'localhost';
+ }
+ else {
+ throw new Error('Invalid URL scheme \'' + u.protocol + '\' specified.');
+ }
+
+ if (!agent._events || agent._events['upgrade'].length === 0) {
@pgriess
pgriess added a note May 24, 2011

If an Agent already has listeners for the 'upgrade' event, do we just silently fail to work?

Not re-using an Agent here allows getting rid of this work-around.

@indexzero
Owner
indexzero added a note May 25, 2011

Yes, it will silently fail. This is the same edge-case I'm seeing in node-http-proxy that is prompting me to rethink how I instantiate agents.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ agent.on('upgrade', (function() {
+ var data = undefined;
+
+ return function(res, s, head) {
+ stream = s;
+
+ //
+ // Emit the `wsupgrade` event to inspect the raw
+ // arguments returned from the websocket request.
+ //
+ self.emit('wsupgrade', httpHeaders, res, s, head);
+
+ stream.on('data', function(d) {
+ if (d.length <= 0) {
+ return;
+ }
+
+ if (!data) {
+ data = d;
+ } else {
+ var data2 = new buffer.Buffer(data.length + d.length);
- httpClient.on('upgrade', (function() {
- var data = undefined;
-
- return function(req, s, head) {
- stream = s;
+ data.copy(data2, 0, 0, data.length);
+ d.copy(data2, data.length, 0, d.length);
- stream.on('data', function(d) {
- if (d.length <= 0) {
- return;
- }
+ data = data2;
+ }
- if (!data) {
- data = d;
- } else {
- var data2 = new buffer.Buffer(data.length + d.length);
+ if (data.length >= 16) {
+ var expected = computeSecretKeySignature(key1, key2, challenge);
+ var actual = data.slice(0, 16).toString('binary');
- data.copy(data2, 0, 0, data.length);
- d.copy(data2, data.length, 0, d.length);
+ // Handshaking fails; we're donezo
+ if (actual != expected) {
+ debug(
+ 'expected=\'' + str2hex(expected) + '\'; ' +
+ 'actual=\'' + str2hex(actual) + '\''
+ );
- data = data2;
- }
+ process.nextTick(function() {
+ // N.B. Emit 'wserror' here, as 'error' is a reserved word in the
+ // EventEmitter world, and gets thrown.
+ self.emit(
+ 'wserror',
+ new Error('Invalid handshake from server:' +
+ 'expected \'' + str2hex(expected) + '\', ' +
+ 'actual \'' + str2hex(actual) + '\''
+ )
+ );
+
+ if (self.onerror) {
+ self.onerror();
+ }
+
+ finishClose();
+ });
+ }
- if (data.length >= 16) {
- var expected = computeSecretKeySignature(key1, key2, challenge);
- var actual = data.slice(0, 16).toString('binary');
+ //
+ // Un-register our data handler and add the one to be used
+ // for the normal, non-handshaking case. If we have extra
+ // data left over, manually fire off the handler on
+ // whatever remains.
+ //
+ stream.removeAllListeners('data');
+ stream.on('data', dataListener);
- // Handshaking fails; we're donezo
- if (actual != expected) {
- debug(
- 'expected=\'' + str2hex(expected) + '\'; ' +
- 'actual=\'' + str2hex(actual) + '\''
- );
+ readyState = OPEN;
process.nextTick(function() {
- // N.B. Emit 'wserror' here, as 'error' is a reserved word in the
- // EventEmitter world, and gets thrown.
- self.emit(
- 'wserror',
- new Error('Invalid handshake from server:' +
- 'expected \'' + str2hex(expected) + '\', ' +
- 'actual \'' + str2hex(actual) + '\''
- )
- );
+ self.emit('open');
- if (self.onerror) {
- self.onerror();
+ if (self.onopen) {
+ self.onopen();
}
-
- finishClose();
});
- }
-
- // Un-register our data handler and add the one to be used
- // for the normal, non-handshaking case. If we have extra
- // data left over, manually fire off the handler on
- // whatever remains.
- //
- // XXX: This is lame. We should only remove the listeners
- // that we added.
- httpClient.removeAllListeners('upgrade');
- stream.removeAllListeners('data');
- stream.on('data', dataListener);
-
- readyState = OPEN;
- process.nextTick(function() {
- self.emit('open');
-
- if (self.onopen) {
- self.onopen();
+ // Consume any leftover data
+ if (data.length > 16) {
+ stream.emit('data', data.slice(16, data.length));
}
- });
-
- // Consume any leftover data
- if (data.length > 16) {
- stream.emit('data', data.slice(16, data.length));
}
- }
- });
- stream.on('fd', fdListener);
- stream.on('error', errorListener);
- stream.on('close', function() {
- errorListener(new Error('Stream closed unexpectedly.'));
- });
-
- stream.emit('data', head);
- };
- })());
- httpClient.on('error', function(e) {
- httpClient.end();
+ });
+ stream.on('fd', fdListener);
+ stream.on('error', errorListener);
+ stream.on('close', function() {
+ errorListener(new Error('Stream closed unexpectedly.'));
+ });
+
+ stream.emit('data', head);
+ };
+ })());
+ }
+
+ agent.on('error', function (e) {
errorListener(e);
});
- var httpReq = httpClient.request(httpPath, httpHeaders);
-
+ var httpReq = protocol.request({
+ host: u.hostname,
+ method: 'GET',
+ agent: agent,
+ port: u.port,
+ path: httpPath,
+ headers: httpHeaders
+ });
+
httpReq.write(challenge, 'binary');
httpReq.end();
})();
@@ -596,4 +635,4 @@ setConstant(WebSocket.prototype, 'OPEN', OPEN);
setConstant(WebSocket.prototype, 'CLOSING', CLOSING);
setConstant(WebSocket.prototype, 'CLOSED', CLOSED);
-// vim:ts=4 sw=4 et
+// vim:ts=4 sw=4 et

1 comment on commit db0cce0

@indexzero
Owner

I agree with everything you've said, but I really don't have the time to make the updates for a couple of weeks (or more). It works for our testing purposes in node-http-proxy and the bulk of the work is done.

Sorry I can't go the extra mile here, hope this still makes it in with some edits on your end.

Please sign in to comment.
Something went wrong with that request. Please try again.