PolyEdge / scratchapi

[CURRENTLY UNMAINTAINED] Scratch API Interface
40 stars 15 forks source link

No timeout for socket connections #16

Open TheLogFather opened 7 years ago

TheLogFather commented 7 years ago

I'm finding that, occasionally, when Scratch Cloud has problems, the cloud socket will just hang when making a request.

I'm assuming this is because there's no timeout for the socket – and, since it's blocking, it just sits there waiting for a reply that'll never come.

Can we add a self.connection.setdefaulttimeout(15.0), or something, just after it creates the socket? (I've added it to mine, but I've yet to see the situation arise again, so don't know for sure that it fixes the problem I see with it hanging occasionally when cloud goes bad – but don't know what else it could be...)

PolyEdge commented 7 years ago

Since you're more up-to-date about the scratch cloud, I've invited you as a collaborator. If you accept the invitation, you can push changes, and (hopefully) help me rewrite the cloud interface once the scratch cloud overhaul is finished.

Is this ok?

TheLogFather commented 7 years ago

Yes, it'll be interesting to see if there are any changes to the cloud API after the "longer-term solutions" spoken of in https://github.com/LLK/scratch-flash/issues/1211#issuecomment-264255576... (I'd expect the "something in place for the short term" won't cause any API changes... probably...)

I think the current status of scratchapi is that it is very usable (apart from this timeout issue, which I now have no way to test while cloud is in fallback-only mode... :/ ), so I don't feel any need to add further contributions in the short term. (Though I think it'd be useful to add some brief CloudSession docs to the wiki – in particular, to note that a BrokenPipeException from a CloudSession indicates the session has closed for some reason, and therefore needs recreating.)

If and when the API changes in the longer-term, then it's possible work will be required...

PolyEdge commented 7 years ago

In the latest version of scratchapi, there shouldn't be BrokenPipeErrors because scratchapi handles session refreshing automatically.

TheLogFather commented 7 years ago

Ah, indeed, I see you're right...

However, I also see that I've commented that out in my own version... :/

IIRC, the reason was because I found it's actually useful to know that the cloud session is failing – more specifically, that it is never not failing – and so it's worth giving up for a while (rather than trying over and over again, never realising the reason there are no updates is 'cos there's never a connection).

It's particularly relevant at the moment, since the cloud is working only through fallback. In my cloud clients, I specifically use that exception, if it keeps happening, to know that port 531 isn't working, and so to leave it for a while. In particular, I recently made the cloud client for the real-time cloud status project actually switch to setting cloudvars via fallback (for a few mins, before briefly trying a CloudSession again).

I guess an option could be possible – so you can choose to receive the exception if you want...? (TBH, though, if someone wants to make a reliable/robust cloud client, I think they do have to be informed in some way that the connection is failing – and it's A Very Bad Thing for scratchapi to be trying again and again and again, with no pause between, and with no limit, if there is no chance of connecting.)

PolyEdge commented 7 years ago

I'm a bit busy right now, I don't have enough time to rewrite, test and push something as big as that like I was able to in the summer when I didn't have much else to do. I'm still sorting out a solution, I'm accepting ideas at the moment. I can't quickly change my program anymore, as I'm still trying to keep it python2 compatible, and something's bound to go wrong somewhere with the number of people using scratchapi.

PolyEdge commented 7 years ago

I didn't intend for my program to get used this much 😝