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

Allow HTTPS long poll requests #3372

Merged

Conversation

@dahjelle
Copy link
Contributor

@dahjelle dahjelle commented Nov 2, 2016

This is a possible fix for outmoded/discuss#395, where longpoll HTTPS requests were getting cancelled before the timeout value passed to server.stop().

Fair warning: I'm not very familiar with Hapi internals nor low-level Node HTTP & socket handling, so I don't have any idea if this fix is appropriate. Any and all suggestions—or a complete rewrite!—more than welcome! My real goal is simply to get this particular issue fixed in whatever manner is fastest. :-D

Issue Description

Expected behavior

server.stop({timeout}, callback) should wait for the provided timeout before forcibly closing any pending requests.

Observed behavior

For HTTPS requests only, server.stop immediately forcibly closes pending requests, without waiting for either a response to the request or for the timeout.

Analysis

It looks like connection.js looks for the _isHapiProcessing flag. However, in an HTTPS connection, that flag is undefined.

I think what is happening is that the _dispatch function sets the flag on a TLSSocket instance. But, by the time we are handling the stop in the _stop function, we no longer have access to the TLSSocket, but rather have just a Socket.

(As far as I can tell, a TLSSocket is created from a Socket originally? as some sort of wrapper?)

It appears that we can set the flag on the socket by using the _parent property of a TLSsocket. It's private and undocumented, so I hope there is a better way to do this, but that's all I could find.

@devinivy
Copy link
Member

@devinivy devinivy commented Nov 3, 2016

Good find– for TLS I think we should be listening for secureConnection rather than connection in order to track the same connection/socket that appears on req.

@dahjelle
Copy link
Contributor Author

@dahjelle dahjelle commented Nov 3, 2016

@devinivy Interesting! I'm not clear where you mean, though…in the _dispatch function? in the _stop function? Thanks…I'm still quite unfamiliar with this particular code. :-D

@devinivy
Copy link
Member

@devinivy devinivy commented Nov 3, 2016

I mean in connection.js, where connections are added to this._connections. See around line 155. It's sort of a bummer for us that TLS and non-TLS have slightly different APIs, since for the most part hapi is able to cater to "the standard HTTP API", which might include other servers like node-spdy, node-http2, etc. I'm wondering if simply listening for both events will be okay, or if the connection event actually needs to be ignored when using TLS. I suppose that's okay, as the docs instruct one to set the tls server option to true even when using a custom listener over TLS. Hopefully someone else can weigh-in here, or further investigate.

@dahjelle while it's not my decision and I think that your fix works, I'm guessing hapi wont take this particular fix because it relies on the undocumented _parent field, just as you said. So I'm trying to think of some other solutions! Thank you for bringing it up and providing a solution– it definitely illuminates the problem at hand!

@dahjelle
Copy link
Contributor Author

@dahjelle dahjelle commented Nov 3, 2016

I'm guessing hapi wont take this particular fix because it relies on the undocumented _parent field, just as you said.

Definitely makes sense! My objective was to clearly communicate the issue as much as anything—I figured that actually fixing it was rather out of the scope of my expertise. :-)

I'm wondering if simply listening for both events will be okay, or if the connection event actually needs to be ignored when using TLS. I suppose that's okay, as the docs instruct one to set the tls server option to true even when using a custom listener over TLS. Hopefully someone else can weigh-in here, or further investigate.

I tried replacing

        this.listener.on('connection', this._onConnection);

in line 155 with

        if (this.settings.tls) {
            this.listener.on('secureConnection', this._onConnection);
        }
        else {
            this.listener.on('connection', this._onConnection);
        }

(…and then the matching set of code down in _stop for removing the listeners.)

While my new test passes just fine that way, it causes another test failure:

Failed tests:

  100) Connection _stop() waits to stop until all connections are closed (HTTPS):

      actual expected

      02

      Expected 0 to equal specified value

      at /…/hapi/test/connection.js:625:95

…which I haven't dug into yet.

Hopefully someone else can weigh-in here, or further investigate.

Hopefully! :-)

@dahjelle
Copy link
Contributor Author

@dahjelle dahjelle commented Nov 3, 2016

I think there is more subtlety here than I'm figuring out. :-/

@dahjelle dahjelle force-pushed the fix-graceful-restarts-for-longpoll-and-https branch from 0f96fe9 to b4ba28a Nov 8, 2016
@@ -622,7 +622,6 @@ describe('Connection', () => {

expect(err).to.not.exist();
expect(count1).to.equal(2);
expect(Object.keys(server.connections[0]._connections).length).to.equal(2);
Copy link
Contributor Author

@dahjelle dahjelle Nov 8, 2016

The way this test is written doesn't take into account the handshake that happens before the secureConnection event is fired. I removed this part of the test to account for that—is that a correct way to fix it?

Copy link
Member

@devinivy devinivy Nov 8, 2016

Try the following,

                const socket1 = new Net.Socket();
                const socket2 = new Net.Socket();
                socket1.on('error', () => { });
                socket2.on('error', () => { });

                socket1.connect(server.info.port, '127.0.0.1');
                socket2.connect(server.info.port, '127.0.0.1');

                TLS.connect({ socket: socket1, rejectUnauthorized: false }, () => {

                    TLS.connect({ socket: socket2, rejectUnauthorized: false }, () => {

                        server.listener.getConnections((err, count1) => {

                            expect(err).to.not.exist();
                            expect(count1).to.equal(2);
                            expect(Object.keys(server.connections[0]._connections).length).to.equal(2);
                            // ...

Copy link
Member

@devinivy devinivy Nov 8, 2016

You should actually create the socket and TLS.connect() one at a time to be safe (rather than creating/connecting both at the same time then calling TLS.connect() in series).

Copy link
Member

@devinivy devinivy Nov 8, 2016

Might look like,

                const socket1 = new Net.Socket();
                const socket2 = new Net.Socket();
                socket1.on('error', () => { });
                socket2.on('error', () => { });

                socket1.connect(server.info.port, '127.0.0.1');
                TLS.connect({ socket: socket1, rejectUnauthorized: false }, () => {

                    socket2.connect(server.info.port, '127.0.0.1');
                    TLS.connect({ socket: socket2, rejectUnauthorized: false }, () => {

                        server.listener.getConnections((err, count1) => {

                            expect(err).to.not.exist();
                            expect(count1).to.equal(2);
                            expect(Object.keys(server.connections[0]._connections).length).to.equal(2);

Copy link
Contributor Author

@dahjelle dahjelle Nov 8, 2016

Awesome, thanks!

@dahjelle
Copy link
Contributor Author

@dahjelle dahjelle commented Nov 8, 2016

I pushed up new changes using listening for either secureConnection or connection depending on whether this.settings.tls is true or not. I had to adjust one of the existing tests to take into account the fact that secureConnection doesn't fire until after TLS handshaking is done…please feel free to make suggestions.

Otherwise, this PR gets away from using the private _parent property, and still fixes the issue I had.

Comments/suggestions very welcome!

@dahjelle dahjelle force-pushed the fix-graceful-restarts-for-longpoll-and-https branch from b4ba28a to 0e868ca Nov 8, 2016
Previously did not pass since the secureConnection event only fired after negotiation. Using `TLS.connect` resolves this.
@dahjelle dahjelle force-pushed the fix-graceful-restarts-for-longpoll-and-https branch from 0e868ca to 375a9a7 Nov 8, 2016
@dahjelle
Copy link
Contributor Author

@dahjelle dahjelle commented Nov 8, 2016

Thanks for your comments, @devinivy ! I adjusted the PR.

@dahjelle dahjelle changed the title Attemped fix for hapijs/discuss#395. Attempted fix for hapijs/discuss#395. Nov 10, 2016
@dahjelle
Copy link
Contributor Author

@dahjelle dahjelle commented Nov 10, 2016

What's next for this PR? Further suggestions?

@dahjelle dahjelle changed the title Attempted fix for hapijs/discuss#395. Fix for hapijs/discuss#395. Nov 10, 2016
@devinivy
Copy link
Member

@devinivy devinivy commented Nov 12, 2016

Looks good to me. Want to get @hueniverse's impression. I would be curious now if today's HTTP2 implementations use the secureConnection event (as they ought to).

@AdriVanHoudt
Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Nov 13, 2016

@devinivy let me /cc @jasnell to answer that since he is working on the http2 implementation

@dahjelle
Copy link
Contributor Author

@dahjelle dahjelle commented Nov 22, 2016

This patch seems to be working for us in production for the past week or so, for whatever that's worth.

@hueniverse hueniverse added the bug label Nov 29, 2016
@hueniverse hueniverse self-assigned this Nov 29, 2016
@hueniverse hueniverse added this to the 16.0.0 milestone Nov 29, 2016
@hueniverse hueniverse merged commit 5c10b22 into hapijs:master Nov 29, 2016
2 checks passed
@hueniverse hueniverse changed the title Fix for hapijs/discuss#395. Allow HTTPS long poll requests Nov 29, 2016
hueniverse added a commit that referenced this issue Nov 29, 2016
@dahjelle dahjelle deleted the fix-graceful-restarts-for-longpoll-and-https branch Nov 29, 2016
@lock
Copy link

@lock lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants