search

FredyH / GWSockets

WebSockets for GLua
MIT License
87 stars 7 forks source link

Closing and removing reference to socket doesn't clean up registry? #21

Closed Tenrys closed 4 years ago

Tenrys commented 4 years ago

I've noticed that when closing a socket and removing the reference to it, if I look through the registry it'll stay there, even after trying to collect garbage. Is this intended?

Also here's a random typo

FredyH commented 4 years ago

I fixed the typo. This code should be responsible for freeing the reference to the socket after it has been closed. I don't believe I hold a reference to it anywhere else, so at some point it should be gced.

notcake commented 4 years ago

We are seeing that the registry slot allocated by ReferenceCreate is never cleared, so ReferenceFree must not be getting run after calling :close().

Tenrys commented 4 years ago

https://github.com/Metastruct/gmod-metaconcord/commit/1e92d74a0a57b88b13857c30d2e92f724beeda54

The change introduced in this commit apparently fixes my problem with user data sticking in the registry, somehow? Is this how?

FredyH commented 4 years ago

Not gonna lie, you really didn't make it easy for me to spot the difference by converting all the indentation to tabs. Do you have any idea which line caused it?

Tenrys commented 4 years ago

I'm sorry, it wasn't intended.

Basically, I made the socket nil itself on disconnect and that makes it clean itself from the registry, in theory.

FredyH commented 4 years ago

I don't see how setting self to nil would actually change anything, since self is just a parameter of the function.

Are you sure that the think hook was running when you were testing? Because the reference can only be freed from there. In the meantime I will try and test it on my server and see if it works correctly there.

FredyH commented 4 years ago

I have actually found a bug where it would leak references. If you close a socket and then reopen it immediately (i.e. from the onDisconnected() callback), it will create a new reference and overwrite the old one in the socketTableReferences.

Was that possibly the cause in your case? If so I will quickly test and then push my changes.

Tenrys commented 4 years ago

I personally wasn't able to reproduce anymore, I don't know if you've tested with cake. Maybe it was something to do with the server itself

FredyH commented 4 years ago

Alright, in that case I will close this for now. If it happens again feel free to reopen the issue.