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

stop geo tracker in error state #13

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

the01
Copy link
Contributor

@the01 the01 commented Aug 9, 2017

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

@cproof
Copy link
Member

cproof commented Aug 9, 2017

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
Copy link
Contributor Author

the01 commented Aug 9, 2017

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
Copy link
Member

cproof commented Aug 11, 2017

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
Copy link
Contributor Author

the01 commented Aug 14, 2017

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
Copy link
Member

cproof commented Aug 24, 2017

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.

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