Closed tresf closed 8 years ago
If we are just setting websocket
to null, then we do not need to individually set the functions to null, they will automatically become undefined
. Just as well, we only need to do this in the onclose
function. If an error occurs that shuts down the connection, both onerror
and onclose
get called.
His keepAlive variable works, but we also need to reset qzConfig.preemptive
alongside the indexes, because they are removed as they return on setup.
@bberenz thanks for the feedback. So the websocket functions/properties should all invalidate automatically with our existing code, right?
How about this approach? #108
Yes, as soon as the websocket closes, any further calls will result in an error caused by a CLOSED state.
Yes, as soon as the websocket closes, any further calls will result in an error caused by a CLOSED state.
setInterval
seems to be a special edge-case where nulling the variable won't stop it from executing. This should help: https://github.com/tresf/qz-print/commit/e64cf68723bf5315c797e49f00d1d51b4625c3bf
From our client:
Just finished testing on the bus on the way home. Attached is my version.
I called cleanup() in the onClose() regardless of any condition to always cleanup the websocket pointer.
And then only call cleanup in onError if we are about to attempt to try the next port or abort. I am assuming that the onError may get called if there is some sort of network error but the socket connection remains open.
And finally in the cleanup I wrapped the call to clearInterval with a check that keepAlive is valid.
(I've incorporated the client's changes here tresf@4f01ec8)
First, I tested the scenario where keepAlive is null and can confirm that clearInterval throws an error, so I believe that check is good. :)
Second, I also agree with the part where a valid websocket throwing an error may not be grounds for a self-destruct alone. In practice this may be a bit different. We'll have to test this thoroughly on all browsers to see how they behave.
Third, logic tells me that this is a typo. Reason I say this is that outOfBounds should always fire a self-destruct, so I'm not sure what was the reasoning behind this line was. @bberenz can you help clear this up?
So, I've committed the changes so far to my personal branch via https://github.com/tresf/qz-print/commit/4f01ec8f0950838d9335e71680e653082a007f5e.
The websocket's onclose
method is called after every failed attempt to connect. That check is there so that qzSocketClose
will only get called after all possible ports have failed to open. Though this doesn't work on secure pages where the loop breaks when trying an insecure port.
This was also set up before we had the qzNoConnection
call, and could be removed if it isn't the behavior you want.
@bberenz sorry, I linked the wrong line... Here: https://github.com/qzind/qz-print/blob/1.9/js/qz-websocket.js#L88
I believe that one was added for the same reason, but because onerror
is called first and does the incrementing itself, that check is never acted upon.
Ok, I won't beat it up further since 2.0 rewrites a lot of this code anyway. I'll squash and merge these changes and get them tested prior to the 1.9.5. roll-out. Thanks for the review.
@bberenz we have a client requesting to call
deployQZ()
a second time after the tray software has been started.These are the proposed changes to the
qz-websocket.js
:I wonder if there is a better approach to this. I've already reset the counter via https://github.com/qzind/qz-print/commit/bf513472067730ad9c7395c6ec70dffe88fe7290, but the above concerns seem to be pretty valid about cleaning up the lingering resources.
Here are the client's proposed diffs...
qz-websocket.js