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

Redirecting from socket hangs with large data #2323

Closed
LinusU opened this issue Aug 7, 2015 · 8 comments
Closed

Redirecting from socket hangs with large data #2323

LinusU opened this issue Aug 7, 2015 · 8 comments
Labels
net Issues and PRs related to the net subsystem. stream Issues and PRs related to the stream subsystem.

Comments

@LinusU
Copy link
Contributor

LinusU commented Aug 7, 2015

I also posted this to nodejs bug tracker (nodejs/node-v0.x-archive#25823), but the issue is also present in iojs 3.0.0 so it felt appropriate to report it here as well

I've been spending lots of time tracking this down for multer, and form-data/multipart file uploader for express. Basically what we want to do is; whenever there is an error 1) unpipe the request 2) .resume() the request so that the browser will consider the request done.

This is implemented something like the code below. req is an http request from a browser.

function abort (err) {
  req.unpipe(parser)
  req.resume()
  next(err)
}

It has been working great, most of the times. It seems like there is a problem when the stream contains too much data. For some reason it just doesn't emit any more data events, and the browser is stuck on uploading (xx%).

I've managed to track this down and have a small test-case that is easily replicated. The problem goes all the way down to net.Socket.

Calling req.resume after socket.pipe(out2) doesn't help.

If the size of each buffer (560000) is lowered it starts working. The break point on my machine is at 16 KiB, a.k.a 1024 * 16 doesn't work, but 1024 * 16 - 1 works.

The test case

var net = require('net')
var stream = require('stream')

var port = 5433
var app = net.createServer()

function logStream (name) {
  var out = new stream.Writable()

  out._write = function (chunk, encoding, cb) {
    console.log('[server][' + name + '] received chunk of data')
    this.emit('chunk-received')
    cb(null)
  }

  return out
}

app.on('connection', function (socket) {
  var out1 = logStream('out 1')
  var out2 = logStream('out 2')

  out1.once('chunk-received', function redirect () {
    console.log('[server] redirecting to `out 2`')
    socket.unpipe(out1)
    socket.pipe(out2)
  })

  socket.pipe(out1)
})

app.listen(port, function () {
  var socket = new net.Socket({ allowHalfOpen: true })

  socket.connect(port, function () {
    setInterval(function () {
      socket.write(new Buffer(560000))
    }, 100)
  })
})

Actual output

[server][out 1] received chunk of data
[server] redirecting to `out 2`

Expected output

[server][out 1] received chunk of data
[server] redirecting to `out 2`
[server][out 2] received chunk of data
[server][out 2] received chunk of data
[server][out 2] received chunk of data
[server][out 2] received chunk of data
[server][out 2] received chunk of data
[server][out 2] received chunk of data
...
@LinusU
Copy link
Contributor Author

LinusU commented Aug 7, 2015

I've added some longer tests in LinusU/node-stream-bug which shows that 1) the client request isn't closed 2) the problem exists with net, http and express

@mscdex
Copy link
Contributor

mscdex commented Aug 7, 2015

FWIW the expected output occurs on node v0.10.x

@mscdex mscdex added net Issues and PRs related to the net subsystem. stream Issues and PRs related to the stream subsystem. labels Aug 7, 2015
@mscdex
Copy link
Contributor

mscdex commented Aug 7, 2015

I think I have a fix for this. PR coming soon ...

@mscdex
Copy link
Contributor

mscdex commented Aug 7, 2015

@LinusU #2325 should fix this. Can you verify?

@LinusU
Copy link
Contributor Author

LinusU commented Aug 7, 2015

@mscdex Really appreciate the fast response, I'm having limited time and internet connection this weekend but I'll try to review as fast as possible. Thanks for the help!

Side note if you happen to know, is cloning your branch and building the only option, or is there some buildbot for pull requests?

@mscdex
Copy link
Contributor

mscdex commented Aug 7, 2015

@LinusU AFAIK there is no buildbot for PRs, so you will probably have to just compile manually.

@LinusU
Copy link
Contributor Author

LinusU commented Aug 14, 2015

Built it myself and it fixed the issue 👍

@kadishmal
Copy link

When will this PR be merged? Node 0.10.x fails.

mscdex added a commit to mscdex/io.js that referenced this issue Oct 11, 2015
If a pipe is cleaned up (due to unpipe) during a write that
returned false, the source stream can get stuck in a paused state.

Fixes: nodejs#2323
@mscdex mscdex closed this as completed in 6899094 Oct 11, 2015
Trott pushed a commit to Trott/io.js that referenced this issue Oct 12, 2015
If a pipe is cleaned up (due to unpipe) during a write that
returned false, the source stream can get stuck in a paused state.

Fixes: nodejs#2323
PR-URL: nodejs#2325
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
mscdex added a commit that referenced this issue Oct 12, 2015
If a pipe is cleaned up (due to unpipe) during a write that
returned false, the source stream can get stuck in a paused state.

Fixes: #2323
PR-URL: #2325
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Issues and PRs related to the net subsystem. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

No branches or pull requests

3 participants