TgSharp icon indicating copy to clipboard operation
TgSharp copied to clipboard

Fix CI and unstable transport

Open aarani opened this issue 3 years ago • 22 comments

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

aarani avatar May 12 '21 13:05 aarani

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

omerjacobi avatar May 13 '21 09:05 omerjacobi

@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.

aarani avatar May 13 '21 15:05 aarani

@aarani New transport will work through proxy?

studasd avatar May 13 '21 15:05 studasd

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

aarani avatar May 13 '21 15:05 aarani

if it's only to be able to use .NETStandard, you can use 4.7.1

2 The versions listed here represent the rules that NuGet uses to determine whether a given .NET Standard library is applicable. While NuGet considers .NET Framework 4.6.1 as supporting .NET Standard 1.5 through 2.0, there are several issues with consuming .NET Standard libraries that were built for those versions from .NET Framework 4.6.1 projects. For .NET Framework projects that need to use such libraries, we recommend that you upgrade the project to target .NET Framework 4.7.2 or higher.

Source

aarani avatar May 17 '21 22:05 aarani

Source

I was suggesting 4.7.1 because it's what we use in geewallet and works fine. Anyway this is a good source, please add the URL to the commit message.

knocte avatar May 18 '21 03:05 knocte

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

aarani avatar May 18 '21 15:05 aarani

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.

knocte avatar May 18 '21 16:05 knocte

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.

Laiteux avatar May 18 '21 16:05 Laiteux

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.

knocte avatar May 18 '21 17:05 knocte

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 avatar May 18 '21 18:05 aarani

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.

aarani avatar May 18 '21 18:05 aarani

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.

Laiteux avatar May 19 '21 08:05 Laiteux

CI is broken now in your branch.

knocte avatar May 19 '21 13:05 knocte

CI is broken now in your branch.

because my session is now broken

aarani avatar May 19 '21 19:05 aarani

because my session is now broken

Let's fix that before you change the retryCount.

knocte avatar May 20 '21 00:05 knocte

any update on this PR?

omerjacobi avatar May 24 '21 07:05 omerjacobi

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

knocte avatar May 26 '21 07:05 knocte

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 avatar May 26 '21 08:05 aarani

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

aarani avatar May 26 '21 08:05 aarani

any update?

omerjacobi avatar Jun 29 '21 13:06 omerjacobi

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.

Laiteux avatar Sep 08 '22 11:09 Laiteux