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

Introduce XMPP pipelining. #1181

Merged
merged 1 commit into from
Mar 2, 2017
Merged

Introduce XMPP pipelining. #1181

merged 1 commit into from
Mar 2, 2017

Conversation

kzemek
Copy link
Contributor

@kzemek kzemek commented Feb 8, 2017

This PR introduces pipelining via updating exml to a version that can self-reset XMPP streams, allowing to parse payloads with stream:stream tag and XML declaration in the middle.

@@ -360,8 +360,6 @@ wait_for_feature_request({xmlstreamelement, El}, StateData) ->
end,
if

Choose a reason for hiding this comment

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

According to Elvis:

Replace the 'if' expression on line 361 with a 'case' expression or function clauses.

@@ -317,7 +303,7 @@ replace_stream_ns(Element, _State) ->
Element.

get_parser_opts(<<"<open", _/binary>>) -> [{infinite_stream, true}, {autoreset, true}]; % new-type WS

Choose a reason for hiding this comment

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

According to Elvis:

Line 305 is too long: get_parser_opts(<<"<open", _/binary>>) -> [{infinite_stream, true}, {autoreset, true}]; % new-type WS.

{ok, Req, State};
websocket_info(reset_stream, Req, #ws_state{parser = Parser} = State) ->
{ok, NewParser} = exml_stream:reset_parser(Parser),
{ok, Req, State#ws_state{ parser = NewParser, open_tag = undefined }};
websocket_info({set_ping, Value}, Req, State = #ws_state{ping_rate = none}) when is_integer(Value) and (Value > 0)->

Choose a reason for hiding this comment

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

According to Elvis:

Line 114 is too long: websocket_info({set_ping, Value}, Req, State = #ws_state{ping_rate = none}) when is_integer(Value) and (Value > 0)->.

@@ -360,8 +360,6 @@ wait_for_feature_request({xmlstreamelement, El}, StateData) ->
end,
if

Choose a reason for hiding this comment

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

According to Elvis:

The expression on line 361 and column 21 is nested beyond the maximum level of 3.

@kzemek kzemek force-pushed the pipelining branch 2 times, most recently from fca63fc to 402358f Compare February 22, 2017 12:59
@kzemek kzemek closed this Feb 22, 2017
@kzemek kzemek reopened this Feb 22, 2017
@kzemek kzemek changed the title Update exml to a version with dirty NIFs. Introduce XMPP pipelining. Feb 28, 2017
@kzemek kzemek removed the WIP 🚧 label Feb 28, 2017
rebar.config Outdated
@@ -9,7 +9,7 @@
{base16, ".*", {git, "git://github.com/goj/base16.git", "f78918e"}},
{cuesport, ".*", {git, "git://github.com/esl/cuesport.git", "d82ff25"}},
{redo, ".*", {git, "git://github.com/Wallapop/redo.git", "35a8d1c"}},
{exml, ".*", {git, "git://github.com/esl/exml.git", "2.4.1"}},
{exml, ".*", {git, "git://github.com/esl/exml.git", {ref, "7b5bfca"}}},
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you change exml ref to esl/exml@d365533 ?


{shotgun, ".*", {git, "https://github.com/inaka/shotgun.git", "4e67065"}},
{exml, ".*", {git, "git://github.com/esl/exml.git", "7b5bfca"}},
{escalus, ".*", {git, "git://github.com/esl/escalus.git", "8f0e101"}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we need a PR in esl/escalus

Copy link
Contributor

@michalwski michalwski left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. I left some comments. Do you think it would be worth to add some tests for error cases? For example if the authentication fails with pipelining?

@michalwski michalwski merged commit 167e275 into master Mar 2, 2017
@michalwski michalwski deleted the pipelining branch March 2, 2017 10:07
@michalwski
Copy link
Contributor

Thanks!

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.

3 participants