Fix #1512

[indutny]

n/t

See #1558

[indutny]

CI: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/637/

[silverwind]

Rebuilt again with this PR, no crashes 👍

[indutny]

LGTY anyone? cc @shigeki @bnoordhuis @iojs/collaborators

[shigeki]

@indutny I reviewed the patch and made several tests on Win and Linux. The issue was resolved with this. But I still could not figure out the reason why series of InvokeQueued were called in sync when ECONNRESET error only on Windows not on Linux. Do you have any information about this?

[indutny]

Perhaps the roots are in libuv, and the difference in the way it reports error on windows and on unix. /cc @piscisaureus @bnoordhuis @saghul

[indutny]

@shigeki and thank you for taking a look

[saghul]

@indutny can you brief me on the core issue?

[indutny]

Oh right. Basically, uv_write() returns error synchronously more often on windows than it does on unix. The code wasn't well written and didn't handle the synchronous error in a right manner (it should emulate asynchronous behavior in some cases), but this didn't show up anywhere except windows.

[indutny]

I suspect that it could be that windows always invokes the syscall from the main thread, while unix does it off-thread when the queue is not empty. But I can't believe that the queue was full all the time.

Maybe it is about the way unix reports errors like ECONNRESET?

[saghul]

@indutny Yeah. Even when Unix does the write on the spot (because the write queue is empty) the error is reported on the next loop iteration: https://github.com/libuv/libuv/blob/v1.x/src/unix/stream.c#L1381

On Windows, however we don't queue the writes, the kernel does. If the call to WSASend fails, however, we return the error immediately. Can you try this (untested) patch? https://gist.github.com/saghul/e05b99b1b16a7c9c15e9

At any rate, I'd say we should handle this in libuv, uv_write should only return the following errors:

• ENOMEM: we couldn't copy the buffer structures
• EBADF: the stream was closed or not bound, and same goes for the handle we're sneding, if any
• ENOBUFS: if we're trying to send nothing

[silverwind]

@saghul unfortunately, it fails to compile:

  tcp.c
src\win\tcp.c(872): error C2039: 'u' : is not a member of 'uv_write_s' [C:\git\iojs\deps\uv\libuv.vcxproj]
          C:\git\iojs\deps\uv\include\uv.h(468) : see declaration of 'uv_write_s'
src\win\tcp.c(874): error C2039: 'stream' : is not a member of 'uv_tcp_s' [C:\git\iojs\deps\uv\libuv.vcxproj]
          C:\git\iojs\deps\uv\include\uv.h(490) : see declaration of 'uv_tcp_s'

[saghul]

Ah, that patch was done against latest libuv, I'll make one against 1.4.2 for you to test.

[saghul]

@silverwind here is a patch which should compile with libuv 1.4.2: https://gist.github.com/saghul/52677dba652ab8e78051

[silverwind]

@saghul thanks, the patch works fine and looks to fix the crashes too.

[saghul]

@silverwind Fantastic! I'll open an issue over at libuv so we can land this properly.

[saghul]

libuv/libuv#339

[indutny]

I suggest landing this PR anyway. @silverwind, @shigeki LGTY?

[silverwind]

This is a critical bug, so I'm fine with landing a temporary fix for it, to be removed in the next libuv release. Can't really review the code, sorry.

[indutny]

This is not a temporary fix. The error wasn't properly handed anyway, it is the bad libuv behavior the exposed the problem, but it was here for a long time anyway.

[saghul]

@indutny The fix looks kinda obvious, but a LGTM from @piscisaureus would be nice.

[piscisaureus]

@saghul https://gist.github.com/saghul/e05b99b1b16a7c9c15e9 lgtm

[indutny]

@piscisaureus and this one? ^

[saghul]

@indutny it's the same fix. We changed the internal structure to not use an anonymous union.

[indutny]

@saghul I meant reviewing this PR...

[piscisaureus]

@indutny 5992187 lgtm although I don't understand what you mean by "In fact, TLS was doing exactly this thing which led us to...".

[indutny]

@piscisaureus the commit log is borked, I'll rewrite it. Thanks for the review!

[shigeki]

@saghul Thanks for the patch. I really understand this behavior on Windows. It is very helpful for me.

@indutny I have one more question. Do sync/async checks need per WriteItem? Because most of InvokeQueued are called in async, if InvokeQueued is called in sync only when an error returns from uv_write(), can we also invoke ``InvokeQueuedin async viauv_check`? Like

diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc
index c774a84..1bd1f05 100644
--- a/src/tls_wrap.cc
+++ b/src/tls_wrap.cc
@@ -117,6 +117,23 @@ bool TLSWrap::InvokeQueued(int status) {
 }


+void TLSWrap::CheckInvokeQueued::CheckCb(uv_check_t* check) {
+  CheckInvokeQueued* c = reinterpret_cast<CheckInvokeQueued*>(check->data);
+  c->w_->InvokeQueued(c->status_);
+  uv_close(reinterpret_cast<uv_handle_t*>(check), nullptr);
+  delete c;
+}
+
+
+int TLSWrap::InvokeQueuedAsync(int status) {
+  CheckInvokeQueued* check = new CheckInvokeQueued(this, status);
+  uv_check_init(env()->event_loop(), &check->check_);
+  check->check_.data = check;
+  int err = uv_check_start(&check->check_, CheckInvokeQueued::CheckCb);
+  return err;
+}
+
+
 void TLSWrap::NewSessionDoneCb() {
   Cycle();
 }
@@ -311,7 +328,7 @@ void TLSWrap::EncOut() {
   // Ignore errors, this should be already handled in js
   if (err) {
     write_req->Dispose();
-    InvokeQueued(err);
+    InvokeQueuedAsync(err);
   } else {
     NODE_COUNT_NET_BYTES_SENT(write_size_);
   }
diff --git a/src/tls_wrap.h b/src/tls_wrap.h
index 9f09535..de7140f 100644
--- a/src/tls_wrap.h
+++ b/src/tls_wrap.h
@@ -62,6 +62,22 @@ class TLSWrap : public crypto::SSLWrap<TLSWrap>,
   // Maximum number of buffers passed to uv_write()
   static const int kSimultaneousBufferCount = 10;

+  class CheckInvokeQueued {
+  public:
+  CheckInvokeQueued(TLSWrap* w, int status) : w_(w), status_(status)  {
+    }
+
+    ~CheckInvokeQueued() {
+      w_ = nullptr;
+    }
+
+    static void CheckCb(uv_check_t* check);
+
+    TLSWrap* w_;
+    int status_;
+    uv_check_t check_;
+  };
+
   // Write callback queue's item
   class WriteItem {
    public:
@@ -88,6 +104,7 @@ class TLSWrap : public crypto::SSLWrap<TLSWrap>,
   void ClearOut();
   void MakePending();
   bool InvokeQueued(int status);
+  int  InvokeQueuedAsync(int status);

   inline void Cycle() {
     // Prevent recursion

I've just borrowed your code. If it is right, I think it can reduce the costs to check that if every WriteItem are in async or sync.

[saghul]

I might be completely off here... but is this really needed if libuv does The Right Thing (TM)? If uv_write only fails with the errors I mentioned here: #1563 (comment) then, from iojs' prespective, those are programming errors and you could abort(). In all other cases the callback would be called in the future, as expected.

[shigeki]

@saghul Yes, I agree it. After fixing libuv, only ENOMEM, EBADF or ENOBUFS returns from uv_write() so that we need not to save in the sync case.

[indutny]

Hm... if you insist on it.... Let's land the first commit then.

[indutny]

Any objections?

[shigeki]

No, objection. LGTM.

[indutny]

Landed in 30b7349, thank you!

[indutny]

I suggest landing the second part of this changeset too, at least until it'll be fixed in libuv.

[indutny]

cc @saghul @shigeki @Fishrock123

[indutny]

PTAL

[saghul]

The patch will land as is plus a test on libuv, so I guess you could float
it.
On May 5, 2015 16:24, "Fedor Indutny" [email protected] wrote:

PTAL

—
Reply to this email directly or view it on GitHub
#1563 (comment).

[silverwind]

@indutny should we revert 30b7349 now that the libuv update is here in #1646?

[indutny]

@silverwind nope, it is good to have anyway.

[silverwind]

Alright!