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

Use default port 443 for wss:// #22

Closed
wants to merge 1 commit into from

Conversation

edubart
Copy link

@edubart edubart commented Jun 5, 2017

This fixes #21

Copy link
Member

@agentzh agentzh left a comment

Choose a reason for hiding this comment

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

Please add some corresponding tests to cover this change in the existing test suite as well. Thank you!

@@ -91,7 +92,11 @@ function _M.connect(self, uri, opts)
-- ngx.say("port: ", port)

if not port then
port = 80
if str_sub(uri, 1, 6) == 'wss://' then
Copy link
Member

Choose a reason for hiding this comment

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

The string.sub() builtin creates a new Lua string, which is a new GC object, which incurs extra GC overhead and dynamic allocation overhead on this potentially hot code path. Better use the ngx.re.find() builtin to do the matching without creating any new GC objects.

@edubart
Copy link
Author

edubart commented Jun 6, 2017

Changes were made to use re_find instead of string.sub.

@edubart
Copy link
Author

edubart commented Jun 6, 2017

The testing suite is not working well in my environment (many tests failing) and I cannot fix it because I am not familiar with perl, I won't be able to provide such tests because of this is extra work. Again, I will leave using my fork.

@agentzh
Copy link
Member

agentzh commented Jun 6, 2017

@edubart You don't need to know perl to get this working. Have you seen our tutorial on the testing toolchain here?

https://openresty.gitbooks.io/programming-openresty/content/testing/

Also, you'll find the Travis CI configuration for this project very helpful:

https://github.com/openresty/lua-resty-websocket/blob/master/.travis.yml

This project has been using Travis CI to do regression testing on every commit.

@agentzh
Copy link
Member

agentzh commented Jun 6, 2017

@edubart This is just like you don't need to know C to use the Linux kernel :)

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.

Incorrect default port for wss
2 participants