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

Fails on node v4 #104

Closed
hueniverse opened this issue Sep 9, 2015 · 9 comments
Closed

Fails on node v4 #104

hueniverse opened this issue Sep 9, 2015 · 9 comments
Assignees
Labels
Milestone

Comments

@hueniverse
Copy link
Member

@hueniverse hueniverse commented Sep 9, 2015

> lab -i 22

_http_server.js:515
  this._handle.readStart();
              ^

TypeError: Cannot read property 'readStart' of null
    at Socket.onSocketResume (_http_server.js:515:15)
    at emitNone (events.js:67:13)
    at Socket.emit (events.js:166:7)
    at resume_ (_stream_readable.js:712:10)
    at doNTCallback2 (node.js:429:9)
    at process._tickDomainCallback (node.js:384:17)

Also 84 and 85.

@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Sep 9, 2015

I was able to reproduce this issue without wreck... here is the issue:

var Http = require('http');
var Fs = require('fs');
var Url = require('url');

var server = Http.createServer(function (req, res) {

    res.writeHead(200);
    res.end();
});


server.listen(8080);

var uri = Url.parse('http://localhost:8080/');
uri.method = 'post';
var req = Http.request(uri);



var stream = Fs.createReadStream(__dirname + '/images/wreck.png');
stream.pipe(req);

There seems to be a limit around 16kb on the size of payloads that work vs break.

@kanongil

This comment has been minimized.

Copy link
Member

@kanongil kanongil commented Sep 9, 2015

This seems like a node issue, though not something that is likely to happen under normal circumstances.

As far as I can tell, it happens if the reply is processed before the request is completed. In your example it can be "fixed" by scheduling res.end() on process.nextTick().

@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Sep 9, 2015

I can change the tests to be under 16kb when sending a payload, which will fix the tests.

@kanongil

This comment has been minimized.

Copy link
Member

@kanongil kanongil commented Sep 9, 2015

This fixes all tests for me:

diff --git a/test/index.js b/test/index.js
index 74a1dea..d70ba13 100755
--- a/test/index.js
+++ b/test/index.js
@@ -535,17 +535,20 @@ describe('request()', function () {
         var gen = 0;
         var server = Http.createServer(function (req, res) {

-            if (!gen++) {
-                res.writeHead(307, { 'Location': '/' });
-                res.end();
-            }
-            else {
-                res.writeHead(200, { 'Content-Type': 'text/plain' });
-                Wreck.read(req, null, function (err, res2) {
+            process.nextTick(function () {

-                    res.end(res2);
-                });
-            }
+                if (!gen++) {
+                    res.writeHead(307, { 'Location': '/' });
+                    res.end();
+                }
+                else {
+                    res.writeHead(200, { 'Content-Type': 'text/plain' });
+                    Wreck.read(req, null, function (err, res2) {
+
+                        res.end(res2);
+                    });
+                }
+            });
         });

         server.listen(0, function () {
@Marsup

This comment has been minimized.

Copy link
Member

@Marsup Marsup commented Sep 9, 2015

Isn't it related to nodejs/node#2355 ?

@hueniverse

This comment has been minimized.

Copy link
Member Author

@hueniverse hueniverse commented Sep 9, 2015

The real question is if this indicates a problem with node v4 or just something minor we can fix in our own tests. The existing failing code looks legit to me.

@Marsup

This comment has been minimized.

Copy link
Member

@Marsup Marsup commented Sep 9, 2015

@geek and @lloydbenson talked to some core contributors at nodeconfeu and should be working on a test case soon, that's probably up to node to fix it.

@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Sep 11, 2015

I logged an issue here: nodejs/node#2821

@geek geek added the test label Sep 11, 2015
@geek geek added this to the 6.2.1 milestone Sep 11, 2015
@geek geek self-assigned this Sep 11, 2015
@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Sep 11, 2015

Workaround for issue is in 6633ff5

@geek geek closed this Sep 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.