V1.3.8 handle exception gracefully when a url doesn't resolve #63

sanakanchha · 2018-07-03T15:37:09Z

Whenever a url doesn't resolve(due to network connectivity, incorrect url etc.), this lib throws an exception which wasn't handled gracefully.

coveralls · 2018-07-03T15:40:15Z

Coverage decreased (-0.4%) to 94.875% when pulling a7b3000 on v1.3.8 into d5197da on develop.

brozeph · 2018-07-03T16:04:21Z

lib/key.js

+		// handle any uncaught exceptions
+		.catch((err) => {
+
+			return err.message;


This should be return reject(err);

Shouldn't it be return Promise.reject(err)? I think reject is outside of the scope..

Yes, good catch!

I had it this way initially. Unfortunately, the unhandled promise rejection didn't get fixed killing the realtime app.

However, I could either go with Promise.resolve(err); or just the way it is in order to send a graceful message to the realtime app.

maybe add a catch for the yield ensureAuthHeaders call here and then if it catches the error emit the error?

if ensureAuthHeaders fails, the co should bubble the exception as a rejected Promise. However, notice that the co function is not being returned... add a return statement before co and this will probably help.

or rewrite a bit...

self.connect = (socketEventSubscriber) => (co(function *() { // teh codez herr });

okay, working on a fix. on the player.js file, thank you.
We found an issue with socket-io-client where it doesn't try to reconnect if failed during during authorization. Craig is looking into whether upgrading socket-io-client is a solution.

Here is the link: https://stackoverflow.com/questions/24390482/socket-io-failing-authorization-does-not-retry-to-reconnect

brozeph · 2018-07-03T16:06:03Z

test/lib/key.js

@@ -279,7 +279,7 @@ describe('key', () => {
 	});

 	describe('#ensureAuthHeaders', () => {
-		it('should require clientId', (done) => {


If you remove the callback here but don't use async and await then the test won't run / complete the way it is intended to. If you'd like to head this route, the better step is to upgrade the mocha dependency, but this is a relatively in-depth task because there may need to be a lot more adjustments made in the SDK to support this.

brozeph · 2018-07-03T16:06:52Z

test/lib/key.js

 				});
 		});

-		it('should require secret', (done) => {


Same comment as above - this is not having the intended effect, the test is not running.

cmerchant · 2018-07-05T17:58:18Z

lib/player.js

-						clientId = authHeaders[CLIENT_ID];
-
-					socket.io.opts.query = ['clientId=', clientId, '&', 'token=', authToken].join('');
+			if(authHeaders != null) {


This condition will always be true because you are always returning an error from the call to ensureAuthHeaders or it actually returns headers

also, you would never try to connect at all meaning the reconnect logic won't be hit at all

cmerchant · 2018-07-05T18:00:30Z

lib/player.js

+
+					err = JSON.parse(err);
+					if(err.code == 401) {
+					socket.disconnect();


indentation is off here

sanakanchha · 2018-07-05T22:04:59Z

@brozeph , @cmerchant , @rmomii the same PR with new changes if you could take a look would be great.

brozeph · 2018-07-05T22:08:35Z

lib/player.js

+			options = {};
+		}
+
+		if (validation.isEmpty(socketEventSubscriber) || !socketEventSubscriber.emit) {


This works, but we're really not checking to see if the socketEventSubscriber is an EventEmitter per se, only that it has a property named emit and not even that it's a function. Might be worthwhile thinking through how to validate if the emit is a function as well.

if ([ validation.isEmpty(socketEventSubscriber), validation.isEmpty(socketEventSubscriber.emit), typeof socketEventSubscriber.emit !== 'function' ].some((val) => val)) { throw new Error('socketEventSubscriber must be an instance of event.EventEmitter'); }

taken care of

brozeph · 2018-07-05T22:09:31Z

lib/player.js

+		clientId = authHeaders[CLIENT_ID];
+
+		url = [protocol, '://', options.host || settings.host].join('');
+		query = ['clientId=', clientId, '&', 'token=', authToken].join('');


query = ['clientId=', clientId, '&token=', authToken].join('');

taken care of

brozeph · 2018-07-05T22:11:43Z

lib/player.js

+		socket.on('reconnecting', (attempt) => {
+			let connection = {
+				'connectionAttempt' : attempt,
+				'headerReset' : false,


How is headerReset used by clients... there is also an isReconnect that's present on other connection objects emitted and this field isn't consistent with those. Is this by design and is this well understood by the consuming code? Seems like this could lead to some confusion...

it is used by realtime app. It should be documented in the sdk readme

the main reason is that we are emiting a connected event for both connect and reconnect. It lets the consumer be aware of this situation. In the realtime app case, it is used for logging

I think you can get rid of headerReset. That seems to be a relic of an earlier implementation. @deepakdahal please double check that before removing.

@cmerchant looks like we have tests added for headerReset https://github.com/PlayNetwork/playnetwork-sdk-nodejs/blob/develop/test/lib/player.js#L216

The tests are there, but do we actually use it anywhere? If not, we should remove it and the tests...

okay, headerReset has been removed both from test and code

cmerchant · 2018-07-05T22:15:08Z

lib/player.js

+		authToken = authHeaders[AUTH_TOKEN];
+		clientId = authHeaders[CLIENT_ID];
+
+		url = [protocol, '://', options.host || settings.host].join('');


Will it ever be possible that options.host and settings.host are both defined. What would happen in this case in forming the url?

options.host should take precedent over settings.host if this were to be the case... the idea here is that you configure the SDK at initialization, but you have the ability to override on a per call basis for über flexibility

cmerchant · 2018-07-05T23:20:12Z

I thought there was some co function that needed a return value. Where is that?

brozeph

Getting close!

brozeph · 2018-07-06T15:40:19Z

lib/player.js

+  			validation.isEmpty(socketEventSubscriber),
+  			validation.isEmpty(socketEventSubscriber.emit),
+  			typeof socketEventSubscriber.emit !== 'function'
+			].some((val) => val)) {


de-indent the line 😺

taken care of

brozeph · 2018-07-06T15:41:21Z

lib/player.js

+		socket.on('reconnecting', (attempt) => {
+			let connection = {
+				'connectionAttempt' : attempt,
+				'headerReset' : false,


The tests are there, but do we actually use it anywhere? If not, we should remove it and the tests...

sanakanchha · 2018-07-06T17:38:43Z

@brozeph @cmerchant I should have covered everything :)

cmerchant · 2018-07-06T17:44:49Z

Looks good to me

sanakanchha added 2 commits July 2, 2018 13:45

handle any uncaught exception

6d9611c

update package version

c719cbc

sanakanchha assigned rmomii Jul 3, 2018

brozeph suggested changes Jul 3, 2018

View reviewed changes

sanakanchha force-pushed the v1.3.8 branch from 0d5f215 to c719cbc Compare July 5, 2018 15:56

sanakanchha added 4 commits July 5, 2018 09:00

remove the catch clause

efaf65a

handle no network scenario gracefully

f1fdee4

return error

eee761f

reconnect if failed during authorizaton

e7dcc52

cmerchant reviewed Jul 5, 2018

View reviewed changes

lib/player.js Outdated

err = JSON.parse(err);

if(err.code == 401) {

socket.disconnect();

Copy link

Contributor

cmerchant Jul 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation is off here

add reconnect logic plus make the sdk consumable

a7af97f

brozeph reviewed Jul 5, 2018

View reviewed changes

cmerchant reviewed Jul 5, 2018

View reviewed changes

sanakanchha added 2 commits July 5, 2018 15:18

address cr comment

2d1db2f

address code review comment

cf5fbda

brozeph suggested changes Jul 6, 2018

View reviewed changes

sanakanchha added 4 commits July 6, 2018 09:57

remove conflicting 'headerReset'

1c3ab06

fix test

ca3cf72

fix indentation

340e253

wipe resetHeader completely

a7b3000

brozeph merged commit f4de53a into develop Jul 6, 2018

brozeph deleted the v1.3.8 branch July 6, 2018 17:48

V1.3.8 handle exception gracefully when a url doesn't resolve #63

V1.3.8 handle exception gracefully when a url doesn't resolve #63

Conversation

sanakanchha commented Jul 3, 2018

coveralls commented Jul 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanakanchha commented Jul 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmerchant commented Jul 5, 2018

brozeph left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanakanchha commented Jul 6, 2018

cmerchant commented Jul 6, 2018

coveralls commented Jul 3, 2018 •

edited