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

snode communication and logging clean up #2 & AppDotNot minor refactor #988

Merged
merged 17 commits into from
Mar 20, 2020

Conversation

neuroscr
Copy link

@neuroscr neuroscr commented Mar 19, 2020

overall:

  • make sure NODE_TLS_REJECT_UNAUTHORIZED is set correctly with the research Jeff did

snode stuff:

  • increase random node pool size to 1024 to match mobile (trello has a card on this)
  • sendToProxy now uses agent instead of the TLS global hack
  • handle 401s with a retry, handle 500s with a delayed retry
  • make sure getRandomSnodeAddress can't stack
  • minor refactoring and internal function renaming
  • logging improvements
  • allow http-resource pollServer disconnect to be correct even if it's been stopped before

adn stuff:

  • refactor functions to be useable outside of class, make it easier to integrate with proxy/onion library in snode stuff
  • .loki support improvements
  • remove pollForChannelOnce loop, rely on timer instead

@majestrate
Copy link

ideally the code should never ever ever touch NODE_TLS_REJECT_UNAUTHORIZED and should instead properly handle custom tls certs, or alternatively permit http for .loki gtld.

await conversation.updateSwarmNodes(filteredNodes);
return filteredNodes;
}

markRandomNodeUnreachable(snode) {

Choose a reason for hiding this comment

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

The name doesn't seem to suggest that it would return something. I see that randomSnodePool.lenght is only used for logging, why not log inside markRandomNodeUnreachable? Like "Marked {} as unreachable, {} left in the pool". Then you can have another log message one level above telling the reason (which can be different from what I can see).

Copy link
Author

@neuroscr neuroscr Mar 19, 2020

Choose a reason for hiding this comment

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

it mainly to reduce the number of logging messages. I had that way initially but we already log everytime this is called as it's a p. big deal event and the outer context is more important that the fact a node was removed but it is VERY helpful to know how main are left to ensure roll overs are happening as expected.

Copy link

@msgmaxim msgmaxim Mar 20, 2020

Choose a reason for hiding this comment

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

If you don't want two separate log messages, how about we make another call to getRandomSnodePool() to get the size? (Or add another method to just get the size, without potentially calling initialiseRandomPool.)

Copy link
Author

@neuroscr neuroscr Mar 20, 2020

Choose a reason for hiding this comment

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

well both actions need to happen the banning of the node and getting the remaining count. Is it the name of the function that is the issue?

A compromise maybe the two function solution but seems a bit inefficient, since banning the node needs to happen and knows the count. But I guess it is just a member variable it needs to count, so not as bad as the unreachableNode node situation.

I'll just split for now. Due to the time crunch of trying to get this in for 1.0.5, I'll leave a temp variable in for now and clean it up later.

} else {
log.error('loki_snodes: Giving up trying to contact seed node');
if (snodes.length === 0) {
throw new window.textsecure.SeedNodeError(

Choose a reason for hiding this comment

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

Don't we need to reset this.initialiseRandomPoolPromise here?

Copy link
Author

@neuroscr neuroscr Mar 19, 2020

Choose a reason for hiding this comment

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

Yes

Copy link

@Mikunj Mikunj left a comment

Choose a reason for hiding this comment

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

minor things

js/modules/loki_app_dot_net_api.js Outdated Show resolved Hide resolved
js/modules/loki_app_dot_net_api.js Outdated Show resolved Hide resolved
js/modules/loki_message_api.js Outdated Show resolved Hide resolved
Copy link

@Mikunj Mikunj left a comment

Choose a reason for hiding this comment

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

Looks good

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.

None yet

5 participants