theimo1221 commented 3 years ago

141 Harden PlayNotificationTwo against some errors

Description

This hardens the PlayNotificationTwo functionality against some errors, to ensure other items in queue might be able to continue.

Your checklist for this pull request

🚨 Please review the guidelines for contributing to this repository.

[x] Make sure you are requesting to pull a topic/feature/bugfix branch (right side). Don't request your master!
[x] Make sure you are making a pull request against the beta branch (left side). Also you should start your branch off svrooij/node-sonos-ts/beta.
[x] Check the commit's or even all commits' message styles matches our requested structure.

💔 Thank you!

coveralls commented 3 years ago

Pull Request Test Coverage Report for Build 1147872414

1 of 4 (25.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.05%) to 78.389%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/sonos-device.ts	1	4	25.0%
<!--	Total:	1	4	25.0%	-->

Totals
Change from base Build 1056550956:	-0.05%
Covered Lines:	1461
Relevant Lines:	1866

💛 - Coveralls

svrooij commented 3 years ago

Have you seen how the GetState and RestoreState handle this? https://github.com/svrooij/node-sonos-ts/blob/a6346f2d10be97da1637e3a6d7377046558ec106/src/sonos-device.ts#L352-L413

They skip setting the time if it's a broadcast instead of throwing an error.

theimo1221 commented 3 years ago

Have you seen how the GetState and RestoreState handle this? They skip setting the time if it's a broadcast instead of throwing an error.

Yes I've seen those and had those checks in my function too, but for me SetAVTransportURI is the call which might throw an error, in case e.g. device is shortly unplugged. For restoring the state, this doesn't need to be catched though as it is a one-time-call.

For the whole queue it might be critical, as otherwise, the queue might run into following issue:

First call to PlayNotificationTwo initializes the list, and starts playing
Second call to PlaynotificationTwo adds item to list, as we already have a running queue
Playing the queue crashes (2nd item not played yet)
3+x call to PlayNotificationTwo only adds item to list, as the queue is still running, isn't it?

So, in any scenario where playing the queue crashes hard, the queue won't continue and would never resolve, as new additions would only extend the queue. I might have to surrond the whole playQueue logic with a try catch, which at least restarts/continues playing the queue from next position, so that in the end the queue is resolved and not stuck.

github-actions[bot] commented 3 years ago

:tada: This PR is included in version 2.4.2-beta.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

github-actions[bot] commented 2 years ago

:tada: This PR is included in version 2.5.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

svrooij / node-sonos-ts