refactor: avoid starting servient multiple times

[danielpeintner]

I think it should not be possible to start a servient multiple times. Hence I added a simple check.

Moreover, I think a servient should be usable only once. Hence after a shuthown it is no longer usable.
Anyhow, I am open to other opinions whether we should allow for a re-start again?

fixes #1193

[codecov]

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (b77b16a) 77.59% compared to head (16ec287) 77.57%.

Files	Patch %	Lines
packages/core/src/servient.ts	54.54%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1195      +/-   ##
==========================================
- Coverage   77.59%   77.57%   -0.02%     
==========================================
  Files          83       83              
  Lines       17311    17332      +21     
  Branches     1747     1751       +4     
==========================================
+ Hits        13433    13446      +13     
- Misses       3842     3850       +8     
  Partials       36       36              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[JKRhb]

Thank you, @danielpeintner, the changes already look good to me :) See two comments/suggestions below.

[egekorkan]

Anyhow, I am open to other opinions whether we should allow for a re-start again?

I do not see a use case but I do not see issues for allowing that

[danielpeintner]

Anyhow, I am open to other opinions whether we should allow for a re-start again?

I do not see a use case but I do not see issues for allowing that

The issues I see for allowing to restart is that I fear internal states "hanging around" that cause problems in the end 🙃

[relu91]

I'm not sure I like the approach (should I throw or should I log and silently return). What about:

• In the start method:
  1. throw if the servient was shut down
  2. silently return and debug message the servient was already started if it #started is true
• In the shutdown method:
  1. throw if the servient was not even started (it's an invalid state after all)
  2. silently return and debug message the servient was already shut down #shudown is true

Btw I'm okay with using #shutdown and #started but should we add the private modifier or is it still redundant (given they are really private fields for js)?

[danielpeintner]

Thank you. I wasn't sure either.

In the start method:

1. throw if the servient was shut down

2. silently return and `debug` message the servient was already started if it `#started` is true

I felt it is best to throw if it has been started already since I guess this is an actual error and as a developer I want to know it.

In the shutdown method:

1. throw if the servient was not even started (it's an invalid state after all)

2. silently return and `debug` message the servient was already shut down  `#shudown` is true

Good point... the more I think about I would do the opposite:

• if not started -> throw Error
• if shutdown already -> just debug... does not really harm

Do you have an other opinion? Throw in any case?

Btw I'm okey with using #shudown and #stated but should we add the private modifier or is it still redundant (given they are really private fields for js)?

It is redundant and also shows as such in VS Code.

[relu91]

In my perspective, we should throw if we are modifying the application state into something erroneous. So given the PR scope, the faulty state is when:

• You started the servient -> stopped it -> now you want to start again.
• Fresh new servient -> you stop it

The others should not penalize the developer because:

• if the servient was started and start is called -> you are already in the required state -> nop
• if the servient was stopped and stop is called -> you are already in the required state -> nop

Why do developers want to know if the method is called twice or more?

[danielpeintner]

I am mostly okay with your reasoning.

There is one aspect that is a bit undesirable

• if the servient was started and start is called -> you are already in the required state -> nop

The start method requires us to return a WoT object.
Hence if we want to do what you are suggesting we have to keep a handle.. see the changes in 0d0c1f0

I am fine with doing so.

[danielpeintner]

Note: with the lastest changes we have issues with CoAP tests (see should reject requests for undefined meta operations).
Maybe it is good to get feedback from @JKRhb again

[danielpeintner]

I think we should now have a solution that works for all of us ;-)

[relu91]

Good to go 👍🏻