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

bpd: support idle command and with it MPD 0.14 #3205

Merged
merged 13 commits into from Apr 11, 2019

Conversation

Projects
None yet
2 participants
@arcresu
Copy link
Member

commented Apr 4, 2019

The idle command from a client signals that the connection should remain idle until an event occurs in the player, at which point the client should be notified. This PR adds support for the command by making the client's thread poll a list of events. The server can asynchronously populate the event list in response to requests from other clients.

This is the next step for #800 and gets us to MPD 0.14.

Details from the MPD protocol specification:

Waits until there is a noteworthy change in one or more of MPD’s subsystems. As soon as there is one, it lists all changed systems in a line in the format changed: SUBSYSTEM.

Change events accumulate, even while the connection is not in “idle” mode; no events gets lost while the client is doing something else with the connection. If an event had already occurred since the last call, the new idle command will return immediately.

While a client is waiting for idle results, the server disables timeouts, allowing a client to wait for events as long as mpd runs. The idle command can be canceled by sending the command noidle (no other commands are allowed). MPD will then leave idle mode and print results immediately; might be empty at this time. If the optional SUBSYSTEMS argument is used, MPD will only send notifications when something changed in one of the specified subsytems.

Example excerpt of the log (in the new format introduced in this PR) from an mpDris session:

bpd: <[127.0.0.1:60926]: idle
bpd: <[127.0.0.1:60926]: noidle
bpd: >[127.0.0.1:60926]: OK
bpd: <[127.0.0.1:60926]: currentsong
bpd: >[127.0.0.1:60926]: OK
bpd: <[127.0.0.1:60926]: status
bpd: >[127.0.0.1:60926]: repeat: 0
bpd: >[127.0.0.1:60926]: random: 0
bpd: >[127.0.0.1:60926]: consume: 0
bpd: >[127.0.0.1:60926]: single: 0
bpd: >[127.0.0.1:60926]: playlist: 0
bpd: >[127.0.0.1:60926]: playlistlength: 0
bpd: >[127.0.0.1:60926]: mixrampdb: 0.0
bpd: >[127.0.0.1:60926]: volume: 10
bpd: >[127.0.0.1:60926]: state: stop
bpd: >[127.0.0.1:60926]: OK
bpd: <[127.0.0.1:60926]: idle
bpd: >[127.0.0.1:60926]: ACK [5@0] {} Got command while idle: close
bpd: *[127.0.0.1:60926]: disconnected

(NB: this shows that mpDris sends close when it's quit while in idle mode. Here bpd is replying with an error since it's not expecting that. It didn't matter since the client anyway quit, and the protocol says a client should just disconnect without using close, which is now deprecated.)

Remaining work:

  • See if there's a way to use bluelet here without polling. The only alternative I can think of is to open a new socket we can select and use that to wait for the MPD events.
  • Support the full list of events specified by MPD.
  • Work out when events should be fired (the specification is a bit vague here so this will mean checking the MPD source).
  • Avoid sending duplicate events (use a set for the queued events).
  • Cleanup when clients disconnect while idleing (currently the coroutine stays alive in that case).
  • Add tests.
  • Bump MPD protocol version.
  • Test on real clients besides mpc.

@arcresu arcresu marked this pull request as ready for review Apr 5, 2019

@arcresu

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2019

I've tested this implementation of idle using mpc and mpDris. The latter of these is a realistic client. When interacting with bpd before this PR it repeatedly polls for status, but with these changes it recognises that idle is a supported command and makes use of it. Pausing a song with mpDris makes it issue a noidle followed by a pause command then it goes back into idle mode. It doesn't seem to fully trust the idle mode completely since it periodically breaks out of it and does a manual status request.

There are some subtleties about when events are fired by MPD that we aren't matching here. In practice it doesn't seem to matter since we're sending the events at least as often as MPD does, and redundant events shouldn't cause any problems. For example, the database event is supposed to fire only when update results in an actual change in the music database, but here we just fire it after every update unconditionally.

In terms of the implementation, the way that this command works doesn't seem to fit very elegantly with eventlet's coroutine model. I was initially hoping that I could avoid polling for events and instead rely on a waitable event. I couldn't find a nice way to do that, especially once I realised that the client is allowed to send noidle to cancel the idle mode, or that the client might disconnect while idle. The implementation in the end doesn't seem too messy.

The tests are also a bit hard to manage since they need threading to be able to deal with the blocking request. The tests I've added seem fairly robust, but there's always a chance that they could spuriously fail due to delays in the thread scheduler. If that becomes an issue we can add small delays to fix the tests, or skip them on certain platforms.

arcresu added a commit to arcresu/beets that referenced this pull request Apr 5, 2019

@sampsyo
Copy link
Member

left a comment

Awesome!! Thank you for investigating this, and for trying this out with real MPD clients. 👍

Overall, everything in this design seems copacetic except for the polling mechanism. That's not too bad, of course, and we could totally get by with this mechanism, but I'd love to dive in a little bit to understand why a poll-free (i.e., proper "push") mechanism wasn't feasible with the current Bluelet-based server implementation.

So with apologies for how basic this will be, can you help me understand what would go wrong with this notional implementation?

  • In cmd_idle (i.e., when entering idle mode), just set a flag on Connection: i.e., self.idle = True. And cmd_noidle clears the flag.
  • In Connection.notify, check whether self.idle and, if so, do the subsystem check and self.send out the changed: notifications immediately. Otherwise, queue them up to be released when we next enter idle state.

I feel like I must be missing something big & obvious, so maybe I should just try implementing that to see what goes wrong as an educational exercise. 😃

Show resolved Hide resolved beetsplug/bpd/__init__.py
Show resolved Hide resolved beetsplug/bpd/__init__.py Outdated
Show resolved Hide resolved beetsplug/bpd/__init__.py Outdated
Show resolved Hide resolved beetsplug/bpd/__init__.py Outdated
Show resolved Hide resolved beetsplug/bpd/__init__.py Outdated

arcresu added some commits Apr 4, 2019

bpd: track and log client session details
Keep track of a list of currently-connected clients.

Use `socket.getpeername()` to get an identifier for each connection and
include this in each log message. This function is documented as not
being available on all systems, but it's unclear which systems this
involves.

Also log a message on client connect and disconnect events. If the
disconnection reason is because the client sent a blank line, match MPD
by returning a protocol error then hanging up. Escape curly braces.
bpd: implement the idle command
Getting this command puts the connection into a special mode where it
awaits MPD events (like the player changing state or the playlist
changing due to other clients interacting with the server.

The MPD specification states that events should queue while a client is
connected, and when it issues the `idle` command any matching events
should be sent immediately if there are any, or as soon as they happen
otherwise.

@arcresu arcresu force-pushed the arcresu:bpd-idle branch from b75c027 to e70b213 Apr 8, 2019

@sampsyo

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

To revise the above simplified/maybe-infeasible proposal, in light of the structure of cmd_* handlers you mentioned above:

Instead of calling send directly from the notify method, this could instead just queue up notifications (as it currently does). Instead, the Connection.run method, after running any command, will look through all the connections in the server and dispatch the notifications by calling send. This way, the handlers can remain "string generators" and leave IO to the run method.

@arcresu

This comment has been minimized.

Copy link
Member Author

commented Apr 8, 2019

Thanks for your pointers! I've had a go and managed to get most of the way there. The only remaining problem I can think of is that there's now no way for the player to trigger idle events, which is a necessary feature. That's because the new Server.dispatch_events coroutine is only run at the end of a request, but the player can trigger events independently of any requests.

When the player finishes a song it calls the Server.play_finished() callback, but this isn't bluelet-aware. Any ideas?

@arcresu arcresu force-pushed the arcresu:bpd-idle branch from b2c9266 to 06907cd Apr 8, 2019

@arcresu arcresu force-pushed the arcresu:bpd-idle branch from 06907cd to fa38138 Apr 8, 2019

@sampsyo

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

Aha; that is a very good point! Of course, most of the purpose of doing this in the first place is to be able to send notifications from the player. 😃

Here is a somewhat off-the-wall idea that might work: have the player thread (from finished_callback, for example) send data over a socket to the main plugin loop. That is, the BPD server would spawn a Bluelet "thread" that waits on a special-purpose socket, which only connects locally to a socket on the "other end" that the player writes to. In other words, use a socket for the inter-thread communication from the player to the server. Any chance that might be tractable?

@arcresu

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2019

I played around yesterday and tried two approaches successfully:

  1. Spawning a bluelet thread that just calls dispatch_events every second. It's still polling, but it's less invasive than what I had initially.
  2. Having the player talk to the server over a socket. The difference was that I reused the main socket and had it send a special message that was intercepted before the normal command handling.

I think you're right though that a separate socket would be neater so as not to pollute the real interface. We could even shift over the debugging commands like profile to the new socket too (locally I've also been using a simple nickname command that lets me assign a label to each open connection so that the log messages become a lot easier to read when there are several clients interacting with bpd at once).

bpd: add control socket
A new `ControlConnection` is created each time a client connects over
a new control socket. This is used to forward events from the player,
and also for debugging utilities that are not part of the real MPD
protocol.

This new feature reuses as much infrastructure from the normal protocol
handling as possible (e.g. `Command` for parsing messages). While the
normal connection delegates to server `cmd_*` methods which are string
generators, the control connections delegate to `ctrl_*` methods defined
on the connection itself that are full coroutines.
@arcresu

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2019

Ok I've implemented a second socket server that I'm calling the control server. It delegates to ctrl_* methods on a new ControlConnection class, which are full coroutines instead of string generators. This provides a new home for debug commands. I reused as much of the infrastructure as possible so there's not much additional code complexity. I'm starting to think however that some of these classes ought to be moved into a separate module like beetsplug.bpd.util so that the actual protocol logic and plugin are easier to find in the main file.

I'll keep testing this out with my normal mpDris2/mpdscribble/mpc mixture, but it seems to be working as intended.

We also pick up a new control_port configuration item for bpd. I can't think of any reason why you would want a separate host for the two sockets, but the ports at least need to be different.

@sampsyo
Copy link
Member

left a comment

Awesome!! I'm thrilled this approach seems to be working! 💖 I have one tiny coding suggestion inline, but I think this seems basically ready.

Show resolved Hide resolved beetsplug/bpd/__init__.py Outdated
@sampsyo

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

Looks perfect! Thank you for all your heroic work on this. Is it ready to merge, in your view?

@arcresu

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2019

Thanks for your reviews and ideas :) Yeah I've tried several MPD clients with the new implementation and pushed it around a bit with MPC and netcat, and it seems like everything is behaving well. I think this is good to go.

I had the crazy idea that this new control socket could be a way to disentangle the player from bpd. The MPD clients communicate with bpd on the normal socket which handles all the playlist and library stuff, and then bpd translates it into a very minimal protocol on the control socket. Besides cleaner code, it would allow us to control multiple players in a very natural way (each ControlConnection is either a player or a debug session). Multiple players is a prerequisite for supporting MPD "partitions", but this is a low priority for me since I don't fully understand the use case. Something to think about in the future anyway.

@sampsyo

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

That decoupling is a pretty cool idea! The notion could be that a lower-level MPD-like protocol is useful for direct playback control, whereas the higher-level MPD protocol is what you want for proper user interfaces. Seems worth exploring!

In any case, I'm excited about this initial version to support idle, so I'll merge this now. 🎉 🚢

@sampsyo sampsyo merged commit 818f5bd into beetbox:master Apr 11, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@arcresu arcresu deleted the arcresu:bpd-idle branch Apr 12, 2019

@arcresu arcresu added this to the Modern MPD support milestone Jun 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.