moll / node-mitm

Intercept and mock outgoing Node.js network TCP connections and HTTP requests for testing. Intercepts and gives you a Net.Socket, Http.IncomingMessage and Http.ServerResponse to test and respond with. Super useful when testing code that hits remote servers.
Other
636 stars 48 forks source link

socket.write is synchronous #22

Closed gustavnikolaj closed 8 years ago

gustavnikolaj commented 8 years ago

I would expect the following code, to output:

wrote foo
got bar

but it outputs:

got bar
wrote foo
var Mitm = require('mitm');
var mitm = Mitm();
mitm.on('connection', function (socket) {
    socket
    .on('connect', function () {
        socket.write('foo');
        console.log('wrote foo');
    })
    .on('data', function (chunk) {
        console.log('got', chunk.toString());
    });
});

var socket = require('net').connect(42, 'foobar.com')
socket.on('data', function () {
    socket.write('bar');
});

I think it's very unexpected that write calls it callback synchronously. That is not how it usually works... When working with sockets, at least a tick would go by, before the callback was processed :-)

gustavnikolaj commented 8 years ago

cc @papandreou

moll commented 8 years ago

Thank you for reporting this and sorry for the long wait! This will be in the next release — merged @mwoc's PR and added a few tests to ensure they catch the sync case should someone refactor the code. ;)

moll commented 8 years ago

Would you mind giving it a test, @gustavnikolaj, to confirm it all works as expected? You can install from this repo directly. I'll then release a new version. Thanks!

gustavnikolaj commented 8 years ago

I would love to, but I can't until next week. I'm on vacation at the moment (without a laptop) :-) I'll get back to you as soon as possible.

Den mandag den 21. marts 2016 skrev Andri Möll notifications@github.com:

Would you mind giving it a test, @gustavnikolaj https://github.com/gustavnikolaj, to confirm it all works as expected? You can install from this repo directly. I'll then release a new version. Thanks!

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/moll/node-mitm/issues/22#issuecomment-199263330

gustavnikolaj commented 8 years ago

@moll I can confirm that it's fixed when using the current tip of the master branch :-) :+1: