Fix not closing sockets and bad timeout error condition checks.

[saurtron]

Work done

• Fix not closing sockets consistently at api_analytics.lua and api_spring_launcher.lua.
• Also fixes some error condition checks on client:connect calls.
  • They would result in no error be treated as an error since lua doing not res and not res == "timeout" becomes not res and (not res) == "timeout" (never true) due to operator precedence.

Related issues

• Fixes crashing (maybe on linux only) when launcher not available (see spring-launcher lib leaking sockets when run without launcher Spring-Chobby/Chobby#548).
• Engine crashing when loading chobby without launcher spring#1786

Remarks

• You might want to test this is fine on windows before merging
  • As far as I know the api should work similarly but anyways just in case
• At least on linux this was making spring crash quite fast after starting a game when not run through the launcher.
  • It was running out of descriptors due to not closing the sockets, so after not much, it would start failing to load images, sounds...
• The bad error condition checks probably didn't have much of an effect but it meant ignoring timeout wasn't always working as intended. This might have some unintended side effect tho.
• When connect returns res, error, when there's an error res should always be nil and error be set to some string, still I kept the not res and err ~= "timeout" pattern. I think conditions could be rationally changed to err and err ~= "timeout" but this way feels more solid, and also I think the original developer wanted to do it like this.

[p2004a]

Nice, fixes Spring-Chobby/Chobby#548

[saurtron]

Nice, fixes Spring-Chobby/Chobby#548

Actually, didn't know there's another project. They do have the same code so it needs the same fixes, should I PR to them too?

[p2004a]

Engine can layer archives on top of other archives. In the past BYAR-Chobby was layered on top of Chobby, but then when changes become very large, all of Chobby was merged into BYAR-Chobby repo and this layering disabled.

should I PR to them too?

Really up to you. Maybe that's not really nice of us but so far almost no fixes etc are being ported back to Chobby base. If I wanted to use base Chobby myself for something I would probably do the full diff between our Chobby and base and port stuff one by one...

[saurtron]

I noticed a few more things while doing this, I may work on these after these PRs are resolved, but anyways just for the record.

timeout handling

The timeout thing is implemented in a bit of a fragile way. It doesn't handle timeout reponse at all, sometimes the timeout can be because the server is taking some time to answer, but other times it's because the server just isn't there. At least in my tests seem like it always gives timeout response whatever is going on, at least on api_launcher.

Besides, internally the engine has two different timeout errors, but both get received by the lua side as "timeout", not sure both need to be handled in the same way.

I'm also not sure timeout error is actually recoverable like the lua code expects where maybe later the connect has actually been done.

infinite and quick reconnection attempts

At least on api_launcher, the current (also previous) logic results in the code trying to reconnect in every Update call forever. Not very good. This should be improved to only try to reconnect every few seconds or smth.

checking socket.tcp calls

socket.tcp() is not being checked, but it can actually fail, and when it fails it's a fatal error that usually signals something hairy going on. Maybe that could be handled instead of just letting lua do a "client is nil!" error, which can be obscure to interpret and take long to fix.

close vs shutdown

client:close() vs client:shutdown(), in some cases it may be better to call shutdown before close, or just shutdown, not sure about that. There's a good description at stack overflow but not sure it applies 100% to how luasocket does it.