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.

Sign up for GitHub

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

Jump to bottom

Fixes issue commented on in #377 #501

Merged
merged 3 commits into from
Aug 31, 2011
Merged

Fixes issue commented on in #377 #501

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
63fc15d
style changes
einaros Aug 30, 2011
ae7f253
switched \u**** to \x** for single byte writes, as the former doesn't…
einaros Aug 30, 2011
bd360a1
added text/plain content-type to handshake responses
einaros Aug 31, 2011
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/manager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -775,6 +775,7 @@ Manager.prototype.handleHandshake = function (data, req, res) {
res.writeHead(200, { 'Content-Type': 'application/javascript' });
res.end('io.j[' + data.query.jsonp + '](new Error("' + message + '"));');
} else {
res.writeHead(200, { 'Content-Type': 'text/plain' });
res.writeHead(status);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Two writeHeads ?
  • Why 200 in writeErr? (the other 200 is for jsonp, which we have to do since it has no error handling)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The jsonp response originally had 200, so I kept it for the plaintext
response as well. This isn't as much an http error as a handshake
interpreted by script error, no?

On Aug 31, 2011 5:52 PM, "guille" <
reply@reply.github.com>
wrote:

@@ -775,6 +775,7 @@ Manager.prototype.handleHandshake = function (data,
req, res) {
res.writeHead(200, { 'Content-Type': 'application/javascript' });
res.end('io.j[' + data.query.jsonp + '](new Error%28"' + message +);
} else {

  • res.writeHead(200, { 'Content-Type': 'text/plain' });
    res.writeHead(status);

Two writeHeads ?
Why 200 in writeErr?

Reply to this email directly or view it on GitHub:
https://github.com/LearnBoost/socket.io/pull/501/files#r108484

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's why the jsonp one passes an Error object so that we can do a quick instanceof check to know if the response was successful or not.
For the regular handshake, we of course just inspect the status code. That's why the original has res.writeHead(status), now duplicated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the duplicate writehead is bogus. And you're of course right about the
status code. I'll correct both.

On Aug 31, 2011 7:11 PM, "guille" <
reply@reply.github.com>
wrote:

@@ -775,6 +775,7 @@ Manager.prototype.handleHandshake = function (data,
req, res) {
res.w...
That's why the jsonp one passes an Error object so that we can do a quick
instanceof check to know if the response was successful or not.
For the regular handshake, we of course just inspect the status code. That's
why the original has res.writeHead(status), now duplicated.

Reply to this email directly or view it on GitHub:
https://github.com/LearnBoost/socket.io/pull/501/files#r108609

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks <3

res.end(message);
}
Expand Down Expand Up @@ -808,7 +809,7 @@ Manager.prototype.handleHandshake = function (data, req, res) {
hs = 'io.j[' + data.query.jsonp + '](' + JSON.stringify(hs) + ');';
res.writeHead(200, { 'Content-Type': 'application/javascript' });
} else {
res.writeHead(200);
res.writeHead(200, { 'Content-Type': 'text/plain' });
}

res.end(hs);
Expand Down
4 changes: 2 additions & 2 deletions lib/transports/websocket/default.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,9 +176,9 @@ WebSocket.prototype.write = function (data) {
var length = Buffer.byteLength(data)
, buffer = new Buffer(2 + length);

buffer.write('\u0000', 'binary');
buffer.write('\x00', 'binary');
buffer.write(data, 1, 'utf8');
buffer.write('\uffff', 1 + length, 'binary');
buffer.write('\xff', 1 + length, 'binary');

try {
if (this.socket.write(buffer)) {
Expand Down
6 changes: 3 additions & 3 deletions lib/transports/websocket/hybi-07-12.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,9 +139,9 @@ WebSocket.prototype.write = function (data) {
*/

WebSocket.prototype.frame = function (opcode, str) {
var dataBuffer = new Buffer(str);
var dataLength = dataBuffer.length;
var startOffset = 2
var dataBuffer = new Buffer(str)
, dataLength = dataBuffer.length
, startOffset = 2
, secondByte = dataLength;
if (dataLength > 65536) {
startOffset = 10;
Expand Down