#203 mpg123-1.17.0 does not close HTTP socket FDs on mpg123_close()

[Rajeev V. Pillai]

If you skip around in an HTTP playlist (while not completely playing an file), mpg123-1.17.0 doesn't close any of the old sockets.

The bug is caused by the check for a filename in open_stream() in libmpg123/readers.c. HTTP sockets have no filename associated with them, so the READER_FD_OPENED flag is also not set on these mpg123 handles. So, when mpg123_close() is called for them, stream_close() in libmpg123/readers.c doesn't close the associated FD.

The patch (attached) is to check in open_stream() if the FD given is of a tty, and only for that case not set the READER_FD_OPENED.

Patch tested only on a Linux system.

[Rajeev V. Pillai]

I should clarify that mpg123 doesn't close any HTTP sockets at all on mpg123_close(). Server-closed sockets (ie. completely played files) are left in a CLOSE_WAIT state, and sockets for incompletely played files are left in the ESTABLISHED state.

[Thomas Orgis]

Hm, how did you check this? The HTTP stuff all happens in the main application, not in libmpg123. The check not to close a given file descriptor is intended as-is. As documented for mpg123_open_fd(), when you hand in a file descriptor to use by libmpg123, you are still responsible to clean that up after use.

I thought the code in open_track() and close_track() in src/mpg123.c takes care of that. It sets filept != -1 on http_open() and then closes that.

Hah! If it would get called at all! Oh, dear, when did that happen? Note that you catched the only case where it matters: Jumping between multiple HTTP connections. The usual cases of playing local files and playing only one HTTP stream are not affected. Local files get closed on the next mpg123_open(), implicitly. As you observed, that's not the case for HTTP. The fix should actually insert the call to close_track() where it was intended.

It is time I spent some time on mpg123 ... thanks for catching this one, although I kindly reject your patch, which is against documented behaviour;-)

[Rajeev V. Pillai]

Hm, how did you check this? The HTTP stuff all happens in the main application, not in libmpg123.

I used lsof(8), and strace(1) with "-e connect,close"/"-e open,close".

The check not to close a given file descriptor is intended as-is. As documented for mpg123_open_fd(), when you hand in a file descriptor to use by libmpg123, you are still responsible to clean that up after use.
...
The fix should actually insert the call to close_track() where it was intended.
... thanks for catching this one, although I kindly reject your patch, which is against documented behaviour;-)

D'oh! If you look at the bug report that I tacked on to bug #202, you'll see that close_track() was what I'd originally suggested. Then, when I saw the call only to mpg123_close() in mpg123.c, I decided I was wrong, and then spent a couple of hours trawling through the source code "fixing" the "bug", instead of reading the API docs...

[Thomas Orgis]

OK, got it in trunk now. Thanks for catching this embarrassing omission to call close_track().