panic: send on closed channel after `TypeAuthorizationStateClosed` is received

[true-seeker]

Hi, I encountered a problem with session that was closed.
When TypeAuthorizationStateClosed is received, responses channel being closed

go-tdlib/client/client.go

Line 106 in 154a6df

close(client.responses)

But after that updates are keep getting in

go-tdlib/client/tdlib.go

Line 79 in 154a6df

client.responses <- resp

But the channel is closed so it panics.

Is it a bug or I am missing something?

[true-seeker]

I feel like we need to move the state check from client.go.receiver() to tdlib.go.receiver() and close channel where the writing happens

[zelenin]

it seems to me that no updates should come after AuthorizationStateClosed. How can I reproduce it?

[true-seeker]

You can try to authenticate a session and in the mobile app deactivate it in Privacy and Security settings.
Panic occurs inconsistently, but in a few tries it usually happens.
Log of received updates on my end:

{"@type":"updateAuthorizationState","authorization_state":{"@type":"authorizationStateReady"},"@client_id":1}
{"@type":"updateAuthorizationState","authorization_state":{"@type":"authorizationStateLoggingOut"},"@client_id":1} 
{"@type":"authorizationStateLoggingOut","@extra":"61f1301f-c665-42c6-a612-71c7a02039d6","@client_id":1}
{"@type":"updateAuthorizationState","authorization_state":{"@type":"authorizationStateClosing"}
{"@type":"authorizationStateClosing","@extra":"0c539378-9093-479b-9b53-29b0c86d46c0","@client_id":1}
< a lot of "@type":"authorizationStateClosing">
{"@type":"updateAuthorizationState","authorization_state":{"@type":"authorizationStateClosed"},"@client_id":1}
{"@type":"error","code":500,"message":"Request aborted","@extra":"3dc901fd-7f8d-4bac-b01e-bee330983bcf","@client_id":1}

on the last event It panics

If you can't repeat it, at least consider my MR (but I need to remove return statement first). It will be more "go" style as like channel writer should close it, not the reader.

[zelenin]

"@extra":"3dc901fd-7f8d-4bac-b01e-bee330983bcf"
are there any other entries in the log with this extra? I want to understand better what's going on.
Apparently, before the client closed, a request was executed that did not have time to finish (get a response).

channel writer should close it, not the reader.

receiver is just a dumb daemon that works with tdlib and routes updates to the right client. And all the logic is inside the client

ok, I did this a6b953f

[true-seeker]

No, nothing with 3dc901fd-7f8d-4bac-b01e-bee330983bcf except log with request aborted

Your fix is not enough to prevent panics, because race condition is still possible, case:

1. goroutine tdlib.receiver() check that client.IsClosed = false
2. right before send to the channel scheduler switches to Client.receiver() that closes the channel
3. scheduler switches back to tdlib.receiver() that tries to send to closed channel
4. panic

In that case we need additional mutex (not an idiomatic golang way).

Probably we need to close channel and remove client from clients map[int]*Client in tdlib.receiver() right after ConstructorAuthorizationStateClosed is received?

[zelenin]

In that case we need additional mutex

you're right

Probably we need to close channel and remove client from clients map[int]*Client in tdlib.receiver() right after ConstructorAuthorizationStateClosed is received?

Yes, I've thought about it. But I don't want the client to know about the tdlib instance. maybe shift the responsibility to delete the client to the user? this can help to avoid the problem if the user needs the client for some reason after closing

[true-seeker]

But I don't want the client to know about the tdlib instance

Client won't know about tdlib instance closing channel and removing client from map stays in tdlib.receiver() (like in my PR https://github.com/zelenin/go-tdlib/pull/162/files#diff-bf8f19ea44ca2e0f429dd6019222a33c34f5c88c724e14e5c95b47bc27cfc535R85)

maybe shift the responsibility to delete the client to the user

so channel will be closed only when user explicitely calls client.close() (or smth like that)? It could work, yeah

[zelenin]

Client won't know about tdlib instance closing channel and removing client from map stays in tdlib.receiver() (like in my PR

yes, but the instance handles the client events that I want to handle in the client.

so channel will be closed only when user explicitely calls client.close() (or smth like that)? It could work, yeah

I'll add the client.IsClosed() with mutex and tdlibInstance.gc(), which will delete closed clients