Additional http headers for websocket connection #47

KSDaemon · 2022-09-29T12:01:08Z

Added possibility to provide additional HTTP headers to the underlying websocket connection
Additional ERROR message AUTHENTICATION_FAILED

… websocket connection

Allow to set HTTP Headers

konsultaner · 2022-09-29T13:13:47Z

I had a quick check. looks good. I'll give it a deep one in about 1.5h. Maybe I'll get to merge it today :)

KSDaemon · 2022-09-29T16:01:48Z

Great! Thnx! Looking forward it!)

lib/src/client.dart

lib/src/transport/websocket/websocket_transport_io.dart

KSDaemon · 2022-10-07T10:30:41Z

@konsultaner Hi! I've added tests. Pls have a look. It's a little bit ugly, but don't want to create more complex flow. But I adopt it if you suggest something else.

KSDaemon · 2022-10-11T08:10:23Z

Gentle reminder about PR :)

KSDaemon · 2022-10-13T09:10:40Z

It took time but seems all is cleared now. Tests are passing. @konsultaner Can you have a look?

konsultaner · 2022-10-13T09:25:55Z

I was on a shorts holiday. Will have a look at it tomorrow morning 🤟

codecov · 2022-10-17T07:55:35Z

Codecov Report

Base: 91.47% // Head: 91.47% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (445a59c) compared to base (4aeaba7).
Patch coverage: 90.00% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff           @@
##           master      #47   +/-   ##
=======================================
  Coverage   91.47%   91.47%           
=======================================
  Files          41       41           
  Lines        2639     2640    +1     
=======================================
+ Hits         2414     2415    +1     
  Misses        225      225

Impacted Files	Coverage Δ
lib/src/message/error.dart	`100.00% <ø> (ø)`
...rc/transport/websocket/websocket_transport_io.dart	`79.24% <75.00%> (+0.39%)`	⬆️
lib/src/client.dart	`80.85% <100.00%> (+0.41%)`	⬆️
lib/src/protocol/session.dart	`88.63% <0.00%> (-0.07%)`	⬇️
lib/src/transport/socket/socket_transport.dart	`77.94% <0.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

konsultaner · 2022-10-17T08:06:39Z

lib/src/client.dart

      try {
        var session = await Session.start(realm, transport,
            authId: authId,
            authRole: authRole,
            authMethods: authenticationMethods,
            reconnect: options.reconnectTime);
        _controller.add(session);
+
+        unawaited(transport.onConnectionLost!.future.then((_) async {
+            // Trying to reconnect only if we were connected and there is


I get the point, but what If the router crashes and auto heals. The there will be only one reconnect. I guess the point was to not reconnect if the credentials were wrong?

Hi @konsultaner! Great that you get to this!
Yeah, original idea was not to reconnect when it is not useful, like with bad credentials.

The code in client.dart is a little bit complicated... Maybe we can refactor this in future :)

So if you know how to resolve this problem in more elegant way — please suggest!

konsultaner · 2022-10-17T08:10:14Z

I really like that you considered a different reconnect strategy. I'm not sure this will work in an auto-healing environment. We may have to consider a more complex reconnect strategy depending on the reason why it didn't work or maybe have it configurable? What do you think @KSDaemon

KSDaemon · 2022-10-17T08:16:49Z

Yeah! Right now lib operates only on transport state to take decision: reconnect or not. But there are cases when session state must be taken into account.

Can you describe a bit more about auto-healing envs? I mean that in general (and as I see it in some wamp clients/libs) when the transport connection is aborted/broken/closed — the session is finished. And new reconnection from client means getting new sessionId. Even if it auto resubscribes to topics, the respective subscriptionIds will be different.

KSDaemon · 2022-10-17T08:25:14Z

So in the first run, there are the next points of connection failure and reconnection:

Transport failures before session is established.
Wamp abort (due to auth or other reasons) before session is established. Wamp error means that server processed client messages and get to some point that resolves in abort. Does auto reconnection fix this? Seems that not.
Transport failures during session. Definitely the most important point to proceed with reconnection (if it is configured)
Wamp failures during session. Seems that this situations should not lead to reconnection. As they happens when something is broken on business logic side (like using feature that is not announced, using serializer that is not supported etc), so autoreconnection won't automatically fix them.

konsultaner · 2022-10-17T08:33:12Z

With auto healing I basically mean either:

Transport failures during session. Definitely the most important point to proceed with reconnection (if it is configured)

or

A router somehow crashes and the devops environment restarts it automatically. <- simple explaination

I totally agree with:

Wamp failures during session. Seems that this situations should not lead to reconnection. As they happens when something is broken on business logic side (like using feature that is not announced, using serializer that is not supported etc), so autoreconnection won't automatically fix them.
Wamp abort (due to auth or other reasons) before session is established. Wamp error means that server processed client messages and get to some point that resolves in abort. Does auto reconnection fix this? Seems that not.

Yeah! Right now lib operates only on transport state to take decision: reconnect or not.

seems like I have to add some more logic to that. I think will will add this during the week. I need the connectanum-dart for one of my projects too. So finally will have a real live application myself. I will have to add some stuff like auto-resubscribe and other things.

Maybe you take this part off this pull request and add an issue I will work on? Or if you are willing to contribute, feel free to add this logic!

KSDaemon · 2022-10-17T08:59:14Z

Okay! Let me then close this PR, I'll make a new one without these changes, also I'll file a new issue about better reconnection handling for you.
In the meantime i'll continue my work on Payload Passthru mode (aka PPT) so look for a new PR later :)
And next big thing is End-2-End Encryption (aka e2ee). So stay tuned!

May be you can add me as a contributor? Just for a convenient way of creating issues, reviews etc?

KSDaemon · 2022-10-17T09:43:11Z

Closing in favor of #49

timeout2x and others added 16 commits May 21, 2021 20:56

Added authRole

535d76c

Added authRole

4771cb5

added an ignore for vscode files

982b098

removed extra authRole assignment

02ae706

Merge branch 'konsultaner:master' into master

ed50d36

stop re-attempting to connect if several severe reasons are provided

5850eb2

bad future bugfix

63d4ad4

migration to dart2 - list<int> vs Uint8List

97eac40

bump pineacl version

81884ad

merged upstream

357ac5d

SOFT-473 - fixed reconnect issues with invalid password

d1fd30f

SOFT-123 - adding authentication failures to stoplist

c0e8d19

Added posibility to provide additional http headers to the underlying…

d60d4c3

… websocket connection

Fixed test runs in GH CI

5120851

Merge pull request #1 from s-things/headers

18ca101

Allow to set HTTP Headers

Merge remote-tracking branch 'upstream/master'

378be7b

konsultaner reviewed Sep 30, 2022

View reviewed changes

lib/src/client.dart Outdated Show resolved Hide resolved

lib/src/transport/websocket/websocket_transport_io.dart Show resolved Hide resolved

Added tests for custom http headers in websocket transport

0931e7b

KSDaemon requested a review from konsultaner October 11, 2022 08:09

Fixed flow: trying to reconnect only if session was established

445a59c

konsultaner reviewed Oct 17, 2022

View reviewed changes

KSDaemon closed this Oct 17, 2022

KSDaemon mentioned this pull request Oct 17, 2022

Better reconnection handling #50

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Additional http headers for websocket connection #47

Additional http headers for websocket connection #47

KSDaemon commented Sep 29, 2022 •

edited

konsultaner commented Sep 29, 2022

KSDaemon commented Sep 29, 2022

KSDaemon commented Oct 7, 2022

KSDaemon commented Oct 11, 2022

KSDaemon commented Oct 13, 2022

konsultaner commented Oct 13, 2022

codecov bot commented Oct 17, 2022

konsultaner Oct 17, 2022

KSDaemon Oct 17, 2022

konsultaner commented Oct 17, 2022

KSDaemon commented Oct 17, 2022

KSDaemon commented Oct 17, 2022 •

edited

konsultaner commented Oct 17, 2022

KSDaemon commented Oct 17, 2022

KSDaemon commented Oct 17, 2022

Additional http headers for websocket connection #47

Additional http headers for websocket connection #47

Conversation

KSDaemon commented Sep 29, 2022 • edited

konsultaner commented Sep 29, 2022

KSDaemon commented Sep 29, 2022

KSDaemon commented Oct 7, 2022

KSDaemon commented Oct 11, 2022

KSDaemon commented Oct 13, 2022

konsultaner commented Oct 13, 2022

codecov bot commented Oct 17, 2022

Codecov Report

konsultaner Oct 17, 2022

Choose a reason for hiding this comment

KSDaemon Oct 17, 2022

Choose a reason for hiding this comment

konsultaner commented Oct 17, 2022

KSDaemon commented Oct 17, 2022

KSDaemon commented Oct 17, 2022 • edited

konsultaner commented Oct 17, 2022

KSDaemon commented Oct 17, 2022

KSDaemon commented Oct 17, 2022

KSDaemon commented Sep 29, 2022 •

edited

KSDaemon commented Oct 17, 2022 •

edited