sochix / TLSharp

Telegram client library implemented in C#
1.01k stars 380 forks source link

Pr/697 review updates #940

Open markwest51 opened 4 years ago

markwest51 commented 4 years ago

updated changes aside from nLog from pr/697

knocte commented 4 years ago

@markwest51 thanks! can you rebase it?

markwest51 commented 4 years ago

@knocte - resolved conflicts that appeared, not sure about removing _ from private vars though and also whether to call async methods prefix, we normally remove that as you can tell whether method is async by await call in preview but up to you

knocte commented 4 years ago

There are still conflicts, did you really rebase? rebasing shouldn't imply new commits.

knocte commented 4 years ago

Nevermind, I had the wrong merge option chosen. There are still things to fix in this PR but before I point them out, I gotta ask: did you test it? does it work well for you?

markwest51 commented 4 years ago

@knocte not tested anything yet, was gonna wait till codebase is ready before doing that

knocte commented 4 years ago

not tested anything yet, was gonna wait till codebase is ready before doing that

but the problem about doing that is that we may break it in the next review iteration hehe

markwest51 commented 4 years ago

@knocte that's fine :-), feel free to point out the stuff you see, will amend later after work

knocte commented 4 years ago

that's fine :-)

it's not fine, believe me, finding regressions in code that was working before, is tricky :P

Please test it first, and I will continue with the review once we know this in fact brings value.

markwest51 commented 4 years ago

update event now gets triggered on each update, for test need to figure out how to read saved messages.

Tasks.

  1. Possibly add cancellation token to MainLoopAsync
  2. Figure out how to trigger scheduled tasks/idle tasks from a client - make sure main thread is not blocked
  3. Remove NLog

Is @merqlove able to provide input as it is their original working code.

knocte commented 4 years ago

Apparently there still are conflicts.

markwest51 commented 4 years ago

@knocte where are these conflicts ?, updates work fine from what i can see when running for the last hr, have added cancellation token to methods, inside the main one there are subscribed tasks and idle tasks not sure what the intention of this was.

knocte commented 4 years ago

Github says "This branch cannot be rebased due to conflicts Rebasing the commits of this branch on top of the base branch cannot be performed automatically due to conflicts encountered while reapplying the individual commits from the head branch."

knocte commented 4 years ago

@markwest51 how have you rebased?

markwest51 commented 4 years ago

@knocte i rebased to a previous commit on my branch to remove some information, did i need to rebase to master or something

knocte commented 4 years ago

I guess so, otherwise Github would not let me merge.

markwest51 commented 4 years ago

think i resolved conflicts by rebasing thru GitBash to master, looks like VS2017 is not that good with GitHub, do not merge yet though until tested again

markwest51 commented 4 years ago

@knocte it is back working ok now - getting all updates coming through, was having problems earlier but run a send message test which gave me a socket error about previous connection being closed and then it is back working fine, not sure what that would be but it looked like socket connection was not working, was fine before all the rebasing so could be that.

knocte commented 4 years ago

was having problems earlier but run a send message test which gave me a socket error about previous connection being closed and then it is back working fine, not sure what that would be but it looked like socket connection was not working, was fine before all the rebasing so could be that.

Man, everytime you face a problem like this, please, paste the ex.ToString() here. I cannot comment on such a small level of detail.

knocte commented 4 years ago

Github is still telling me This branch cannot be rebased due to conflicts Rebasing the commits of this branch on top of the base branch cannot be performed automatically due to conflicts encountered while reapplying the individual commits from the head branch. I would recommend to rebase going back to basics: with git commands. Just do git fetch --all and git rebase origin/master (provided origin points to TLSharp, not to your fork of TLSharp)

knocte commented 4 years ago

BTW if it's easier for you, before rebasing you could squash your commits into 1, first. That would make rebasing easier I guess

markwest51 commented 4 years ago

@knocte are you able to check conflicts now, squashed commits using gitbash in the end rather than VS

markwest51 commented 4 years ago

@knocte have done the git commands specified, says branch is up to date and rebased to your master as asked.

knocte commented 4 years ago

ok seems no conflicts now! will do the last review today

merqlove commented 4 years ago

Hello guys, I am not finished that integration in time reasons. I have used for my needs another project in another programming language, which has those features out of the box. Good luck! 🙏

markwest51 commented 4 years ago

@knocte have done the review changes but i still get socket timeout errors and have got the exception back which i mentioned before. Get a lot of socket timeout errors once they go it is fine. @merqlove any insight on what the scheduled and idle tasks was about or any errors you may have got running the updates. ?

Here is stack trace of exception i occasionally get

Test Name: GetUpdatesForUser Test FullName: TLSharp.Tests.TLSharpTestsVS.GetUpdatesForUser Test Source: C:\Users\Admin\Source\Repos\TLSharp2\TLSharp.Tests.VS\TLSharpTestsVs.cs : line 84 Test Outcome: Failed Test Duration: 0:00:00.3699959

Result StackTrace:
at System.Net.Sockets.Socket.BeginReceive(Byte[] buffer, Int32 offset, Int32 size, SocketFlags socketFlags, AsyncCallback callback, Object state) at System.Net.Sockets.NetworkStream.BeginRead(Byte[] buffer, Int32 offset, Int32 size, AsyncCallback callback, Object state) --- End of inner exception stack trace --- at System.Net.Sockets.NetworkStream.BeginRead(Byte[] buffer, Int32 offset, Int32 size, AsyncCallback callback, Object state) at System.IO.Stream.<>c.b__43_0(Stream stream, ReadWriteParameters args, AsyncCallback callback, Object state) at System.Threading.Tasks.TaskFactory1.FromAsyncTrim[TInstance,TArgs](TInstance thisRef, TArgs args, Func5 beginMethod, Func3 endMethod) at System.IO.Stream.BeginEndReadAsync(Byte[] buffer, Int32 offset, Int32 count) at System.IO.Stream.ReadAsync(Byte[] buffer, Int32 offset, Int32 count, CancellationToken cancellationToken) at TLSharp.Core.Network.TcpTransport.<Receive>d__6.MoveNext() in C:\Users\Admin\Source\Repos\TLSharp2\TLSharp.Core\Network\TcpTransport.cs:line 61 --- End of stack trace from previous location where exception was thrown --- at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at System.Runtime.CompilerServices.ConfiguredTaskAwaitable1.ConfiguredTaskAwaiter.GetResult() at TLSharp.Core.Network.MtProtoSender.d14.MoveNext() in C:\Users\Admin\Source\Repos\TLSharp2\TLSharp.Core\Network\MtProtoSender.cs:line 163 --- End of stack trace from previous location where exception was thrown --- at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at System.Runtime.CompilerServices.ConfiguredTaskAwaitable`1.ConfiguredTaskAwaiter.GetResult() at TLSharp.Core.TelegramClient.d24.MoveNext() in C:\Users\Admin\Source\Repos\TLSharp2\TLSharp.Core\TelegramClient.cs:line 108 --- End of stack trace from previous location where exception was thrown --- at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at System.Runtime.CompilerServices.TaskAwaiter.GetResult() at TLSharp.Tests.TLSharpTests.d57.MoveNext() in C:\Users\Admin\Source\Repos\TLSharp2\TLSharp.Tests\TLSharpTests.cs:line 386 --- End of stack trace from previous location where exception was thrown --- at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at System.Runtime.CompilerServices.TaskAwaiter.GetResult() at TLSharp.Tests.TLSharpTestsVS.d12.MoveNext() in C:\Users\Admin\Source\Repos\TLSharp2\TLSharp.Tests.VS\TLSharpTestsVs.cs:line 86 --- End of stack trace from previous location where exception was thrown --- at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at System.Runtime.CompilerServices.TaskAwaiter.GetResult() Result Message: Test method TLSharp.Tests.TLSharpTestsVS.GetUpdatesForUser threw exception: System.IO.IOException: Unable to read data from the transport connection: An established connection was aborted by the software in your host machine. ---> System.Net.Sockets.SocketException: An established connection was aborted by the software in your host machine Result StandardOutput:
Debug Trace: CodeToAuthenticate: {0} not configured in app.config! Some tests may fail. PasswordToAuthenticate: {0} not configured in app.config! Some tests may fail. NotRegisteredNumberToSignUp: {0} not configured in app.config! Some tests may fail. UserNameToSendMessage: {0} not configured in app.config! Some tests may fail. NumberToGetUserFull: {0} not configured in app.config! Some tests may fail. NumberToAddToChat: {0} not configured in app.config! Some tests may fail.

merqlove commented 4 years ago

@markwest51 I did telegram subscriber to some groups & channels, with ability to retranslate incoming messages into Redis pubsub or other telegram channels. Sorry, I have no idea about that errors. It was so long time ago...

knocte commented 4 years ago

Here is stack trace of exception i occasionally get

If you paste only the stacktrace, we don't see the neither the exception type nor the exception message. Please just catch the exception and paste what you get from its ex.ToString()

markwest51 commented 4 years ago

@knocte the exception is at the bottom,

Test method TLSharp.Tests.TLSharpTestsVS.GetUpdatesForUser threw exception: System.IO.IOException: Unable to read data from the transport connection: An established connection was aborted by the software in your host machine. ---> System.Net.Sockets.SocketException: An established connection was aborted by the software in your host machine

markwest51 commented 4 years ago

@knocte are we all good on this now ?

knocte commented 4 years ago

Oh sorry for the delay, let me double check.

markwest51 commented 4 years ago

@knocte all comments done and reviewed. Have added a comment on the test as have worked out why the updates were not being triggered, it was because the user needed re-authenticating, i was using the session user so there must be a expiration time. Soon as i done that all was working fine

markwest51 commented 4 years ago

@knocte we all good ?

knocte commented 4 years ago

@markwest51 hey Mark, sorry to drop the ball on this. Reason is I wanted to push the Layer Update (to 108) first, and later merge this PR, but I lacked the motivation for very long to finish the Layer update first. Today I've finished that and I pushed it here: https://github.com/nblockchain/TgSharp . In the following days I plan to port this PR to that repo, if you don't beat me to it.