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

tls: re-enable .writev() on TLSWrap #1155

Closed
wants to merge 1 commit into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Mar 14, 2015

Fix the parallel/test-tls-over-http-tunnel.js on Windows by
re-enabling the accidentally disabled .writev() method on TLSWrap.

It appears that there is some subtle issue with shutdown timing and it
manifests itself when the chunks are written in separate packets. This
leads to concurrent shutdown/destroy, which breaks the test.

cc @piscisaureus

It appears that removing writev in io.js prior to StreamBase does not introduce this problem, but this patch fixes the problem without touching the test. I propose landing it, and meanwhile I'll investigate more.

Fix the `parallel/test-tls-over-http-tunnel.js` on Windows by
re-enabling the accidentally disabled `.writev()` method on TLSWrap.

It appears that there is some subtle issue with shutdown timing and it
manifests itself when the chunks are written in separate packets. This
leads to concurrent `shutdown`/`destroy`, which breaks the test.
@indutny indutny mentioned this pull request Mar 14, 2015
@indutny
Copy link
Member Author

indutny commented Mar 14, 2015

UPDATE: I was able to reproduce this problem on io.js prior to StreamBase changes after removing .cork()/.uncork() from _http_outgoing.js. So this is definitely unrelated.

@indutny
Copy link
Member Author

indutny commented Mar 14, 2015

cc @iojs/crypto @bnoordhuis seems to be an obvious change

@piscisaureus
Copy link
Contributor

@indutny @indutny Interesting. What I found is that the number of callbacks that node expects (stream._wriableState.pendingcb) doesn't match what libuv thinks.

@indutny
Copy link
Member Author

indutny commented Mar 14, 2015

This is fine, because they are queued in TLSWrap

On Saturday, March 14, 2015, Bert Belder notifications@github.com wrote:

@indutny https://github.com/indutny @indutny
https://github.com/indutny Interesting. What I found is that the number
of callbacks that node expects (stream._wriableState.pendingcb) doesn't
match what libuv thinks.


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

@piscisaureus
Copy link
Contributor

@indutny

This is fine, because they are queued in TLSWrap

It isn't, because this causes the socket to be destroyed before all writes are flushed (What happens is that pendingcb is 0 in node, but libuv still has two chunks of unwritten data).

@indutny
Copy link
Member Author

indutny commented Mar 14, 2015

Aah, well... This is a TLSWrap bug anyway, not StreamBase

On Saturday, March 14, 2015, Bert Belder notifications@github.com wrote:

@indutny https://github.com/indutny

This is fine, because they are queued in TLSWrap

It isn't, because this causes the socket to be destroyed before all writes
are flushed (What happens is that pendingcb is 0 in node, but libuv still
has two chunks of unwritten data).


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

@indutny
Copy link
Member Author

indutny commented Mar 14, 2015

On a second thought, it is very interesting case. This imbalance is happening because .shutdown() request is performing an uv_write() on TLS socket. Looking into improving this.

@indutny
Copy link
Member Author

indutny commented Mar 15, 2015

@piscisaureus LGTY?

@piscisaureus
Copy link
Contributor

@indutny lgtm!

indutny added a commit that referenced this pull request Mar 15, 2015
Fix the `parallel/test-tls-over-http-tunnel.js` on Windows by
re-enabling the accidentally disabled `.writev()` method on TLSWrap.

It appears that there is some subtle issue with shutdown timing and it
manifests itself when the chunks are written in separate packets. This
leads to concurrent `shutdown`/`destroy`, which breaks the test.

PR-URL: #1155
Reviewed-By: Bert Belder <bertbelder@gmail.com>
@indutny
Copy link
Member Author

indutny commented Mar 15, 2015

Landed in 4eb8810, thank you!

@indutny indutny closed this Mar 15, 2015
@indutny indutny deleted the fix/tls-over-http-on-windows branch March 15, 2015 04:09
@rvagg rvagg mentioned this pull request Mar 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants