Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Websocket Lag #81

Merged
merged 2 commits into from
Nov 13, 2017
Merged

Fix Websocket Lag #81

merged 2 commits into from
Nov 13, 2017

Conversation

cinoan
Copy link
Contributor

@cinoan cinoan commented Nov 12, 2017

#78 introduced an issue where ws requests that don't cause a response to be sent, notably requests related to testing modes, caused the queue to wait for a 2000ms timeout on a response that wasn't coming.

Updated the queue so those requests that don't expect a response just pause the queue for 40ms. This removes the lag and also stops the possibility of test messages being sent faster than they could be responded to - e.g. when swiping around the colour picker. The queue just maintains the latest message of each type so if multiple colour requests are generated in that 40ms it is just the latest one that gets sent when the timer expires.

@penfold42
Copy link
Contributor

penfold42 commented Nov 12, 2017

Maybe everything should be required to reply ?

A simple OK or NO for success/fail.

Then we can avoid special casing in the .js

UPDATE:
I just tried it: penfold42@e4bb8bc

seems to work well

@cinoan
Copy link
Contributor Author

cinoan commented Nov 12, 2017

I've no strong preference either way. If responding to all request you would also need to send a response for T0.

@penfold42
Copy link
Contributor

@forkineye ?

@forkineye forkineye merged commit 7e44a90 into forkineye:master Nov 13, 2017
@forkineye
Copy link
Owner

Just got back in town. I'll take a look at these tomorrow after work and get familiar with them, thanks!

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

Successfully merging this pull request may close these issues.

3 participants