Fix CI and unstable transport #41

aarani · 2021-05-12T13:18:09Z

Recently CI has been extremely buggy and unreliable, during extensive testing TCP Transport is identified as the culprit.
Errors like

System.InvalidOperationException : Couldn't read the packet length (was 0, expected 4)

that would happen because of force disconnections right after the first encrypted write.
For this reason and because the transport layer was long overdue for a rewrite we changed the transport to an asynchronous websocket transport + aes-ctr obfuscation

omerjacobi · 2021-05-13T09:36:29Z

@aarani do you know why the CI for windows still fails?

aarani · 2021-05-13T15:37:25Z

@aarani do you know why the CI for windows still fails?

nblockchain's CI sercrets needs an update, for checking out the actual CI result check the results on my fork.

studasd · 2021-05-13T15:50:04Z

@aarani
New transport will work through proxy?

aarani · 2021-05-13T15:52:22Z

yeah, It's not implemented but with a simple code change you can use proxies.

738a849 moves loading of "NumberToSendMessage" property to SendMessage test even though there are 3 other tests that use that variable thereby breaking all 3 tests because of NumberToSendMessage being null. This commit moves the load process back to init phase so all 4 tests can use NumberToSendMessage.

src/TgSharp.Core/Auth/Authenticator.cs

src/TgSharp.Core/MTProto/Crypto/AesCtr.cs

src/TgSharp.Core/Network/MtProtoPlainSender.cs

knocte · 2021-05-18T16:17:53Z

Based on @Laiteux's testing, this commit might cause issues in .NET/.NET Core. These platforms are unsupported by us cause TgSharp is a .NET Framework library but I think it makes sense to fix it before merge

We can fix it later when we migrate to .NETCore/.NET5.

Laiteux · 2021-05-18T16:21:26Z

That means I won't be able to keep using TgSharp anymore, since all my projects are .NET 5 or at least .NET Core 3.1. Honestly, who uses .NET Framework anymore...

Or then a second option would be to keep the TcpTransport and let the user decide what he wanna use, but that would introduce more complexity than anything else.

knocte · 2021-05-18T17:43:31Z

I won't be able to keep using TgSharp anymore

I said we will migrate, but no need to do it in this PR. The smallest this PR is the better, and the earlier we can start merging other PRs. We don't want this PR to become a monster.

aarani · 2021-05-18T18:03:44Z

Based on @Laiteux's testing, this commit might cause issues in .NET/.NET Core. These platforms are unsupported by us cause TgSharp is a .NET Framework library but I think it makes sense to fix it before merge

We can fix it later when we migrate to .NETCore/.NET5.

I agree! it's not our bug either, so we either have to try to fix our dependency and PR it or just use another websocket library which we can figure out after this gets merged

aarani · 2021-05-18T18:05:04Z

That means I won't be able to keep using TgSharp anymore, since all my projects are .NET 5 or at least .NET Core 3.1. Honestly, who uses .NET Framework anymore...

Or then a second option would be to keep the TcpTransport and let the user decide what he wanna use, but that would introduce more complexity than anything else.

It's just for this PR not permanent.

src/TgSharp.Core/Network/MtProtoPlainSender.cs

Laiteux · 2021-05-19T08:40:56Z

This will fix the retry logic:

@@ -136,19 +128,20 @@ namespace TgSharp.Core.Network
             await client.SendInstant(finalInit);
         }

-        public async Task ConnectAsync(int retryCount = 5)
+        public async Task ConnectAsync(int retryCount = 0)
         {
-            while (retryCount-- != 0)
+            while (retryCount-- != -1)
             {
                 try
                 {
                     await InitializeClient();
                     break;
                 }
-                catch (Exception ex)
+                catch
                 {
-                    if (retryCount == 1)
-                        throw ex;
+                    if (retryCount == -1)
+                        throw;
+
                     await Task.Delay(1000);
                 }
             }

I don't have access to this repo therefore I can't do the changes myself for now.

As long as we create a new sessionId for every connection, (by randomizing the sessionId) seqNo can start from 0 and we don't have to save it. This prevents errors like code=32 that are caused by wrong seqNo from happening. This commit also removes the previous workaround that was used on CI to prevent these errors cause they are no longer necessary.

knocte · 2021-05-19T13:46:51Z

CI is broken now in your branch.

aarani · 2021-05-19T19:17:06Z

CI is broken now in your branch.

because my session is now broken

knocte · 2021-05-20T00:50:48Z

because my session is now broken

Let's fix that before you change the retryCount.

omerjacobi · 2021-05-24T07:46:09Z

any update on this PR?

- add response type as generic

knocte · 2021-05-26T06:43:57Z

src/TgSharp.Core/Network/Message.cs

+
+namespace TgSharp.Core.Network
+{
+    public class Message


@aarani nit, should this be internal too?

knocte · 2021-05-26T06:46:07Z

src/TgSharp.Core/Network/MtProtoSender.cs

-
-            uint code = messageReader.ReadUInt32();
-            ulong badMsgId = messageReader.ReadUInt64();
+            _ = messageReader.ReadUInt32();


in HandlePong, you have put a comment explaining why this is ignored (_), can you put a comment here too?

all those comments are mostly old I just ignored some of the unused variables

maybe it's easy to figure out from looking at the spec again for a minute?

knocte · 2021-05-26T06:48:11Z

src/TgSharp.Core/app.config

+    <assemblyBinding xmlns="urn:schemas-microsoft-com:asm.v1">
+      <dependentAssembly>
+        <assemblyIdentity name="System.Threading.Tasks.Extensions" publicKeyToken="cc7b13ffcd2ddd51" culture="neutral" />
+        <bindingRedirect oldVersion="0.0.0.0-4.2.0.1" newVersion="4.2.0.1" />


do all TgSharp consumers need this bindingRedirect now?

I don't know to be honest, I don't think so though

if this is the case, this is a big annoyance... I wonder if we can fix it

- Upgrade .NET to 4.7.2 to use .NET Standard dependency Reason: https://docs.microsoft.com/en-us/dotnet/standard/net-standard - Uses obfuscated WebSocket transport instead of TCP - Adds asynchronous response handling

knocte · 2021-05-26T07:56:12Z

Last but not least, I'm having trouble finding the MS docs where it recommends to use underscores in method names.

aarani · 2021-05-26T08:02:10Z

Last but not least, I'm having trouble finding the MS docs where it recommends to use underscores in method names.

which methods are you talking about ?

aarani · 2021-05-26T08:10:47Z

Last but not least, I'm having trouble finding the MS docs where it recommends to use underscores in method names.

I can't find the original documentation for it, but event handler name Object_EventName is a fairly old convention.

https://docs.microsoft.com/en-us/dotnet/desktop/winforms/event-handlers-overview-windows-forms?view=netframeworkdesktop-4.8

also VS creates event handlers with this same exact naming scheme

omerjacobi · 2021-06-29T13:36:17Z

any update?

Laiteux · 2022-09-08T11:20:20Z

any update?

Nope, and there will never be. Because knocte always had a urge to control every single misplaced comma or underscore despite wanting to take the lead on open-source projects. Hopefully one day he understands you can't have a desire to lead people and a compulsive behavior to "optimize" everyone's code. I would have loved too as well, sadly you'll never see this PR being merged, mainly for that reason. Everyone eventually got tired and that's it, that's why this project is dead. Props to @aarani for his crazy work on everything else and from the very beginning, without that dude I probably wouldn't even be earning a living the way I'm doing it today.

aarani mentioned this pull request May 12, 2021

TlSharp I haven't done anything yet, but Client.ConnectAsync is an error sochix/TLSharp#1021

Open

aarani force-pushed the rewritten-transport branch from c036eeb to e840b42 Compare May 13, 2021 16:05

aarani changed the title ~~[WIP] Core,TL,Gen: Transport layer rewrite and bugfixes~~ [DO NOT MERGE] Core,TL,Gen: Transport layer rewrite and bugfixes May 13, 2021

aarani force-pushed the rewritten-transport branch from e840b42 to 702a94a Compare May 13, 2021 16:09

aarani force-pushed the rewritten-transport branch 12 times, most recently from adccfbf to 92b700e Compare May 13, 2021 23:38

This was referenced May 15, 2021

Tests: load NumberToSendMessage in init process #45

Closed

Core: don't load seqNo and sessionId on start #44

Closed