Samsung / libtuv

Asynchronous I/O for IoT.js and embedded system
Apache License 2.0
129 stars 76 forks source link

Fix uv__io_active assertion in stream.c #106

Closed zherczeg closed 6 years ago

zherczeg commented 6 years ago

When uvstream_destroy is called right after uv__tcp_connect, the POLLOUT flag is set. The uvreq_unregister call after the assertion clears this flag so the assertion is moved after this call.

glistening commented 6 years ago

@zherczeg First, I wonder we can move the assertion. Because libuv seems to think it is unsafe to destroy active request. In this case, the underlying I/O event system may require some kind of cleanup for the descriptor in POLLOUT state. For your information, latest version of uv keep the assertion in the same location. Second, if you're sure it is safe, please enclose the change with TUV_something (e.g., TUV_CHANGES or TUV_FIX, ...) so that we can find out our changes by searching TUV when we need to upgrade uv base version.

zherczeg commented 6 years ago
var net = require('net');
net.connect(1328);
setTimeout({});

The code above triggers this in IoT.js. The invalid setTimeout cause an early exit.

Is it a bug in IoT.js rather than libtuv?

hs0225 commented 6 years ago

@zherczeg I agree with @glistening. It seems that POLLIN and POLLOUT should be removed (uv-io should be stopped) before the stream is destroyed to make a little more safe code.

zherczeg commented 6 years ago

Then the code is strange. First it checks that those two flags are not set, and then frees the request if it is ongoing. So the two things are contradict each other. If the assert is correct, the if below it is dead code.

hs0225 commented 6 years ago

@zherczeg But, if this PR is landed, the bug is still not fixed. (I did not run iotjs_uncaught_exception which generates another assertion when testing.)

uncaughtException: TypeError: Bad arguments: callback must be a Function
iotjs: /home/hosung/work/iotjs/github/iotjs/deps/libtuv/src/unix/stream.c:470: uv__stream_destroy: Assertion `!uv__io_active(&stream->io_watcher, POLLIN | POLLOUT)' failed.
Aborted (core dumped)
hs0225 commented 6 years ago

@zherczeg After Samsung/iotjs#1496 is merged, this bug no longer appears. Please check again.