GNOME Bugzilla – Bug 591481
New BaseWebServer class with DaapProxyWebServer implementing it
Last modified: 2010-04-12 06:23:25 UTC
This new abstract class eliminates code duplication and allows the basic, redundant tasks of listening for connections, validating requests, writing responses, etc. to be done in one spot. I am also using this class in my SoC work for the Telepathy extension. Patch coming...
Created attachment 140477 [details] [review] New abstract web server class
Created attachment 141432 [details] [review] Some changes on Neil's patch A few changes on the patch : - Rename the class to BaseHttpServer - Small improvement on the existing code being moved : StringBuidler instead of string concatenation - Make more properties read-only I wasn't able to test this properly, because I'm apparently unable set-up DAAP sharing properly on my system. So if someone cold confirm that DAAP still works with this, it'd be nice ! Aaron, Gabriel, any comments on the new class ?
Bertrand, your updated class is working for me. Tested DAAP and my Telepathy extension.
Comment on attachment 141432 [details] [review] Some changes on Neil's patch Looks good, sorry it took so long to review.
Comment on attachment 141432 [details] [review] Some changes on Neil's patch I went ahead and committed it. Thanks guys!
This change caused a problem for me using the forked-daapd DAAP server. The check on the length parameter in BaseHttpServer.WriteResponseStream causes Banshee to fail when playing tunes that forked-daapd is set to transcode. According to the comments in forked-daapd's httpd streaming function, it can only guess the overall length of the stream before transcoding is complete, so it does not send "Content-Length" in the stream header. This causes the assertion in BaseHttpServer.WriteResponseStream to fail. I have been able to work around this by commenting out the length parameter check in BaseHttpServer.WriteResponseStream. So far, this has not caused any adverse side-effects that I am aware of. I am by no means a programmer, however, and I have no idea if this is the right thing to do or if the fix should really happen somewhere else. I also have not tested with any other DAAP server that does transcoding.
I assume you're talking about this in BaseHttpServer.WriteResponseStream: + if (length < 1) { + throw new ArgumentOutOfRangeException ("length", "Must be > 0"); + } It's always helpful to submit a patch, if possible. You can read here for more information on that: http://banshee-project.org/contribute/write-code/
Really should submit a new bug report too - that specific check is different enough from that this bug was opened to solve.