Skip to content

Impression events don't respect HTTPS_PROXY #122

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

Closed
Geit opened this issue Jun 21, 2018 · 5 comments
Closed

Impression events don't respect HTTPS_PROXY #122

Geit opened this issue Jun 21, 2018 · 5 comments
Assignees

Comments

@Geit
Copy link

Geit commented Jun 21, 2018

Since #98 removed the request library in favour of the stock Node HTTP implementation, the Optimizely SDK no longer supports HTTP(S) Proxies, as Node does not proxy by default (as documented here nodejs/node#15620).

We host our feature branches on a PAAS that requires outbound HTTP connections be proxied: As a result this bug currently causes our application to crash with the error below when calling optimizely.activate

   2018-06-21T16:49:56.60+0100 [APP/PROC/WEB/0] ERR events.js:183
   2018-06-21T16:49:56.60+0100 [APP/PROC/WEB/0] ERR       throw er; // Unhandled 'error' event
   2018-06-21T16:49:56.60+0100 [APP/PROC/WEB/0] ERR       ^
   2018-06-21T16:49:56.60+0100 [APP/PROC/WEB/0] ERR Error: connect ECONNREFUSED 34.228.76.60:443
   2018-06-21T16:49:56.60+0100 [APP/PROC/WEB/0] ERR     at Object._errnoException (util.js:1022:11)
   2018-06-21T16:49:56.60+0100 [APP/PROC/WEB/0] ERR     at _exceptionWithHostPort (util.js:1044:20)
   2018-06-21T16:49:56.60+0100 [APP/PROC/WEB/0] ERR     at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1198:14)

This error can't be caught, because it occurs asynchronously to the main control flow of the application, and thus causes the entire application server to crash.

@mikeproeng37
Copy link
Contributor

Hi! Thanks for the feedback!

We try to make very few assumptions about the environment that the SDK runs on and instead make it easy for the SDK to be configured to fit in where it is used. The issue you describe has to do with our out of the box event dispatcher. What you can do is to override the event dispatcher as described here: https://developers.optimizely.com/x/solutions/sdks/reference/?language=javascript#event-dispatcher

I've dug up our old event dispatcher, which uses the request library. Feel free to copy over that dispatcher to your code and initialize the SDK with it.

@Geit
Copy link
Author

Geit commented Jun 21, 2018

Thanks for the workaround, we'll try implementing that in our development environments.

However, The more important issue here is probably that the default event dispatcher throws an uncatchable exception and crashes the application server. This will occur whenever/if optimizely is unavailable for any reason (i.e. Blocked by a proxy, or some actual downtime) which should probably be fixed regardless.

@mikeproeng37
Copy link
Contributor

mikeproeng37 commented Jun 21, 2018

👍 we will look into it on our end, thanks!

@spencerwilson-optimizely
Copy link
Contributor

spencerwilson-optimizely commented Jun 22, 2018

I think what may be happening is that http/s.request (used here) returns an http.ClientRequest, an EventEmitter that emits an error event. We don't attach a handler for any error event it may emit, so we're in this case:

If an EventEmitter does not have at least one listener registered for the 'error' event, and an 'error' event is emitted, the error is thrown, a stack trace is printed, and the Node.js process exits.

(via https://nodejs.org/api/events.html#events_error_events)

So we should add an error handler immediately after requesting to catch+log any error that might have occurred.

@mikeproeng37
Copy link
Contributor

Once again, thanks @Geit for reporting this! We'll make sure to prevent the crashing, but won't be adding proxy support at this time. As we noted, you can use a custom event dispatcher to address that issue.

tylerbrandt pushed a commit that referenced this issue Jun 25, 2018
- (nodejs) Prevent crash when `http`/`https` emits an error by adding an 'error' listener
- (nodejs) Fix `requestCallback` to conform to its role as a 'response' listener; notably, ensure it is called when the first argument is interpeted as the `http.IncomingMessage`, and _not_ called in the event of an error (as expected by `Optimizely#_sendImpressionEvent`/`Optimizely#track`).

Tested that this version (as 2.1.2-0) _does_ emit the messages expected in the demo app.

Fixes #122 
Fixes #124
tylerbrandt pushed a commit that referenced this issue Jun 25, 2018
- (nodejs) Prevent crash when `http`/`https` emits an error by adding an 'error' listener
- (nodejs) Fix `requestCallback` to conform to its role as a 'response' listener; notably, ensure it is called when the first argument is interpeted as the `http.IncomingMessage`, and _not_ called in the event of an error (as expected by `Optimizely#_sendImpressionEvent`/`Optimizely#track`).

Tested that this version (as 2.1.2-0) _does_ emit the messages expected in the demo app.

Fixes #122
Fixes #124
tylerbrandt pushed a commit that referenced this issue Jun 25, 2018
- (nodejs) Prevent crash when `http`/`https` emits an error by adding an 'error' listener
- (nodejs) Fix `requestCallback` to conform to its role as a 'response' listener; notably, ensure it is called when the first argument is interpeted as the `http.IncomingMessage`, and _not_ called in the event of an error (as expected by `Optimizely#_sendImpressionEvent`/`Optimizely#track`).

Tested that this version (as 2.1.2-0) _does_ emit the messages expected in the demo app.

Fixes #122
Fixes #124
tylerbrandt pushed a commit that referenced this issue Jun 25, 2018
- (nodejs) Prevent crash when `http`/`https` emits an error by adding an 'error' listener
- (nodejs) Fix `requestCallback` to conform to its role as a 'response' listener; notably, ensure it is called when the first argument is interpeted as the `http.IncomingMessage`, and _not_ called in the event of an error (as expected by `Optimizely#_sendImpressionEvent`/`Optimizely#track`).

Tested that this version (as 2.1.2-0) _does_ emit the messages expected in the demo app.

Fixes #122
Fixes #124
# Conflicts:
#	packages/optimizely-sdk/lib/plugins/event_dispatcher/index.node.js
#	packages/optimizely-sdk/lib/plugins/event_dispatcher/index.node.tests.js
tylerbrandt pushed a commit that referenced this issue Jun 25, 2018
* Fix(nodejs/event_dispatcher): update error/resp handlers (#123)

- (nodejs) Prevent crash when `http`/`https` emits an error by adding an 'error' listener
- (nodejs) Fix `requestCallback` to conform to its role as a 'response' listener; notably, ensure it is called when the first argument is interpeted as the `http.IncomingMessage`, and _not_ called in the event of an error (as expected by `Optimizely#_sendImpressionEvent`/`Optimizely#track`).

Tested that this version (as 2.1.2-0) _does_ emit the messages expected in the demo app.

Fixes #122
Fixes #124
tylerbrandt pushed a commit that referenced this issue Jun 25, 2018
* Fix(nodejs/event_dispatcher): update error/resp handlers (#123)

- (nodejs) Prevent crash when `http`/`https` emits an error by adding an 'error' listener
- (nodejs) Fix `requestCallback` to conform to its role as a 'response' listener; notably, ensure it is called when the first argument is interpeted as the `http.IncomingMessage`, and _not_ called in the event of an error (as expected by `Optimizely#_sendImpressionEvent`/`Optimizely#track`).

Tested that this version (as 2.1.2-0) _does_ emit the messages expected in the demo app.

Fixes #122
Fixes #124
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants