search

rtr-nettest / rmbtws

RTR-NetTest Websocket Client
Apache License 2.0
14 stars 11 forks source link

stop geo tracker in error state #13

Open the01 opened 7 years ago

the01 commented 7 years ago

Upon reaching the error state, stop the geo tracker. Prevents it from continueing to run in case of problems.

cproof commented 7 years ago

This Pull seems problematic.

In of TestEnvironment.getGeoTracker returning null, a GeoTracker will generated by the test (WebSockettest.js:220). This (internal) GeoTracker would not be stopped with your code. Furthermore, it could be possible that a client wants to continue tracking with the GeoTracker provided in the TestEnvironment even if a test finished (e.g. in case the connection is re-established again).

What was the problem you wanted to solve? Would it be a sufficient to stop the GeoTracker in case of an error in your errorhandler?

the01 commented 7 years ago

Well, you are stopping the tracker when the measurement is complete (successful) and this was meant to handle the failure condition. Another and probably cleaner approach would be a method to stop/close/free/cleanup anything used/started by the test. The simple goal is not have garbage or dangling object left.

cproof commented 7 years ago

Sorry, you are right. However, can you please make sure, that .stop() is not called more than once (since setState()) can be called more than once), and that the rmbtws-internal geoTracker ("wsGeoTracker") is also stopped on error?

the01 commented 7 years ago

Ok, sounds reasonable.

On a side note, are you certain that the Error state always transitions to the END state? Otherwise the sockets don't get closed. I would propose a stop() method that closes all resources (websockets, trackers,..) with checks to make it idempotent.

cproof commented 7 years ago

You are right, the ERROR state will not always transition into an END state. A (private) 'close' method that closes open sockets and stops the geolocation (if it was initialized by the rmbtw-client and not externally) sounds good, looking forward to the updated pull request.