Skip to content
This repository has been archived by the owner on Dec 5, 2021. It is now read-only.

addresses connection issues #931

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

addresses connection issues #931

wants to merge 8 commits into from

Conversation

solarin
Copy link
Contributor

@solarin solarin commented Apr 8, 2020

! IsConnected is restored as it was before

  • TcpTransport has got a new Connect method which properly initializes the underlying tcpclient, this makes it possible to reconnect in case the tcpClient gets disconnected, without restarting the application or recreating a new TelegramClient object.

! IsConnected is restored as it was before
+ TcpTransport has got a connect method which properly initializes the underlying tcpclient
…d by the server" but the following communication was not succesfully because the session contained dirty information as well as the sendCounter in TcpTransport.

this commit fixes this situation, so a connection can be succesfully re-enstabilished and the communication restarted.
private int sendCounter = 0;
TcpClientConnectionHandler handler;
string address;
Copy link
Collaborator

Choose a reason for hiding this comment

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

address is not reused, so you don't need to create a field for it

Copy link
Collaborator

Choose a reason for hiding this comment

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

my bad, seems it is needed, howevr these 3 new fields can be marked as readonly

ipAddress = IPAddress.Parse(address);
}

public async Task Connect()
Copy link
Collaborator

Choose a reason for hiding this comment

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

if this method is async its name should have the Async suffix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can change, but also send and receive in the same class are async and are not named as such. i suggest to keep the same style.

Copy link
Collaborator

Choose a reason for hiding this comment

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

this is a convention by Microsoft, the creators of C#; if you see any current API that doesn't fit this pattern we should rename it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok then
guys, i think we should push this fix asap, i have been testing on a software which has been running for half a day now, perfectly handling the reconnection.

before this fix, it could last only 30 minutes.


this.apiHash = apiHash;
this.apiId = apiId;
this.handler = handler;
this.dcIpVersion = dcIpVersion;

session = Session.TryLoadOrCreateNew(store, sessionUserId);
session = Session.TryLoadOrCreateNew(this.store, sessionUserId);
Copy link
Collaborator

Choose a reason for hiding this comment

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

can't we just remove this line above? session will be initialized again anyway in ConnectAsync

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unfortunately not, because the following like needs it.

to save one initialization of session we must add extra code in the Connect method

@knocte
Copy link
Collaborator

knocte commented Apr 9, 2020

guys, i think we should push this fix asap, i have been testing on a software which has been running for half a day now, perfectly handling the reconnection.
before this fix, it could last only 30 minutes.

You make it sound like if this PR fixed connection problems. It doesn't. It just allows the developer to reconnect using the same instance of TelegramClient. You could have done the same before by creating a new TelegramClient instance.

@solarin
Copy link
Contributor Author

solarin commented Apr 9, 2020

guys, i think we should push this fix asap, i have been testing on a software which has been running for half a day now, perfectly handling the reconnection.
before this fix, it could last only 30 minutes.

You make it sound like if this PR fixed connection problems. It doesn't. It just allows the developer to reconnect using the same instance of TelegramClient. You could have done the same before by creating a new TelegramClient instance.

true, but still a great improvement

@knocte
Copy link
Collaborator

knocte commented Apr 10, 2020

Hey Mario I wrote an alternative PR with much simpler change, wouldn't that achieve the same as this?: #937

@solarin
Copy link
Contributor Author

solarin commented Apr 10, 2020

Hey Mario I wrote an alternative PR with much simpler change, wouldn't that achieve the same as this?: #937

yes, it looks fine and easier. let me test it for a couple of hours.

@davidcon
Copy link

davidcon commented Aug 19, 2021

! IsConnected is restored as it was before

  • TcpTransport has got a new Connect method which properly initializes the underlying tcpclient, this makes it possible to reconnect in case the tcpClient gets disconnected, without restarting the application or recreating a new TelegramClient object.

Excuse me @solarin I don´t know how to use your new version.
I have downloaded the master branch and I have compiled it.
Then I have try to use IsConnected as it shuld be but it´s always set true.
Do I have to use any new method to know that the client is disconected?
Or should I have to use await client.ConnectAsync(true) in stead of await client.ConnectAsync()?

I have researched some papers and I think that the problem is caused by TCP client. It´s not posible to know if que the socket is open in real time. Maybe usefull an special connection mode based in some kind of keep alive tcp socket:
https://www.py4u.net/discuss/709629

Thank you very much for you time.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants