THRIFT-5710: Stop sharing headers across all instances of transports in nodejs

[ngavalas]

THRIFT-5710

There is an issue with the header transport implementation in nodejs. All TBufferedTransport and TFramedTransport instances across all connections and clients share a single object for their read and write headers. There is a different copy for the two transport types, but within one type, they are shared.

This means that there are both data races and incorrect results possible:

• You can write headers to a request that has written headers but hasn't flushed yet. This is especially troubling if the headers are used for auth, because you're able to mix up requests and auth as the wrong entity.
• You can read headers from other requests. They only clobber each other if they have the same name, but the union of all seen headers is returned in getReadHeaders .

The issue is that we aren't calling the header transport constructor with the new this from the concrete implementations, so the TBufferedTransport.prototype = new THeaderTransport(); line is not properly binding this and causes the object sharing.

You can see this in the two test cases that fail before this PR and succeed now. Two completely unrelated TFramedTransport objects shared read and write results.

I did not create a JIRA ticket yet because I am still waiting on approval, but I wanted to get the PR up ASAP; we are currently experiencing this bug in production.

Note: my editor reformatted those two or three random lines because they had trailing whitespace and/or tabs, which is against the coding style. I can revert them if you want this PR to be clean, but figured I'd leave the changes in since the existing files were incorrectly formatted.

• Did you create an Apache Jira ticket? (Request account here, not required for trivial changes)
• If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
• Did you squash your changes to a single commit? (not required, but preferred)
• Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
• If your change does not involve any code, include [skip ci] anywhere in the commit message to free up build resources.

[ngavalas]

Just to be 100% clear, before this fix, headers would have contained { Parent: "shoobar", Parens: "shoobaz", Trace: "abcde" } because it was reusing the same object as the first test. This is not test-only though and happens in real life.

Similar story for the write test.

[ngavalas]

@Jens-G I'm going to assume that the cross-test failures are not from me since none of them involve nodejs. Safe to assume?

[Jens-G]

Yes, seems so.

[ngavalas]

Sounds good. Thoughts on the bug and the PR? We're blocked on using nodejs Thrift on this, so I'm eager to get this one in.

[ngavalas]

@Jens-G Anything you need from me to review this? Happy to do whatever it takes!

[ngavalas]

Thanks @Jens-G! Can we cut a new npm package as well?

[Jens-G]

Sure, after release (somewhere this summer) as usual there will be a new npm package.