GNOME Bugzilla – Bug 731153
SoupSession::request-started should also be emitted for cached resources
Last modified: 2015-03-02 08:45:52 UTC
When loading a resource from the disk cache, the request-started signal is not emitted. This affects WebKit that uses that signal to record the requestStart timing (see https://dvcs.w3.org/hg/webperf/raw-file/tip/specs/NavigationTiming/Overview.html#dom-performancetiming-requeststart), but also the logger that never shows the requests of cached resources, and the responses have the wrong SoupMessage identifier (it's always 0). You can test this running the soup-cache test with -H to get the logger output, see: < HTTP/1.1 200 OK < Soup-Debug-Timestamp: 1401788569 < Soup-Debug: SoupMessage 0 (0x2867d70) Only cancelled resources might have a 0 id for SoupMessage. And there's no request information about that SoupMessage in the logger output.
Created attachment 277790 [details] [review] soup-session: emit request-started also for resources loaded from the cache The main problem of this approach is that the SoupSocket parameter will be NULL for cached resources. It would be safer to add a new signal cached-request-started or something like that, but we would also need to connect to both signals everywhere. This will only affect libsoup users that are using both request-started signal and the soup-cache feature, and also using the SoupSocket parameter (I bet SoupLogger is the only one :-) )
(In reply to comment #1) > The main problem of this approach is that the SoupSocket parameter will be NULL > for cached resources. It would be safer to add a new signal > cached-request-started or something like that, but we would also need to > connect to both signals everywhere. This will only affect libsoup users that > are using both request-started signal and the soup-cache feature, and also > using the SoupSocket parameter (I bet SoupLogger is the only one :-) ) It's possible that SoupLogger is the only one, but I wouldn't bet much money on it... The other thing is, SoupSocket is slowly being deprecated anyway, and the SoupSession:request-* signals (and the corresponding SoupSessionFeature methods) are the last place where SoupSocket is still exposed in public non-deprecated API. So I'd like to replace those signals with new ones anyway... The new ones could be called message-queued/message-started/message-unqueued (and maybe also message-finished and/or message-restarted? I seem to remember that the current set of signals is sometimes inconvenient.) They wouldn't need to have any socket/connection argument since (a) as you say, most people aren't using it anyway, and (b) SoupMessages keep track of their SoupConnection internally now, so we could expose some sort of soup_message_get_socket/connection() method later if it was needed. Or, an alternative simpler short-term solution: add a request-started signal to SoupCache.
[mass-moving all "UNCONFIRMED" libsoup bugs to "NEW" after disabling the "UNCONFIRMED" status for this product now that bugzilla.gnome.org allows that. bugspam-libsoup-20150210]
There are more inconsistencies between cached and non-cached resources than the request-started. Cached resources switch to FINISHING when responding to the request, not when input stream io has finished, like the other resources. Instead of fixing this, maybe it's better to start with the new signals making sure they are consistent and deprecate the old ones.
(In reply to Dan Winship from comment #2) > (In reply to comment #1) > > The main problem of this approach is that the SoupSocket parameter will be NULL > > for cached resources. It would be safer to add a new signal > > cached-request-started or something like that, but we would also need to > > connect to both signals everywhere. This will only affect libsoup users that > > are using both request-started signal and the soup-cache feature, and also > > using the SoupSocket parameter (I bet SoupLogger is the only one :-) ) > > It's possible that SoupLogger is the only one, but I wouldn't bet much money > on it... > > The other thing is, SoupSocket is slowly being deprecated anyway, and the > SoupSession:request-* signals (and the corresponding SoupSessionFeature > methods) are the last place where SoupSocket is still exposed in public > non-deprecated API. So I'd like to replace those signals with new ones > anyway... > > The new ones could be called message-queued/message-started/message-unqueued > (and maybe also message-finished and/or message-restarted? I seem to > remember that the current set of signals is sometimes inconvenient.) They > wouldn't need to have any socket/connection argument since (a) as you say, > most people aren't using it anyway, and (b) SoupMessages keep track of their > SoupConnection internally now, so we could expose some sort of > soup_message_get_socket/connection() method later if it was needed. > The only one receiving a socket is request-started, why do we need new signals for the others? just for consistency in the names using message-* instead of request-*?
Ah, maybe we don't need to redo all of them then, although having consistent names is good...
(In reply to Dan Winship from comment #6) > Ah, maybe we don't need to redo all of them then, although having consistent > names is good... What about moving request-started to SoupMessage instead? It's a message signal in the end, since non HTTP requests don't emit the request-started signal either, which could be confusing. And it would be consistent with the other SoupMessage signals RESTARTED and FINISHED. Session features could connect to SoupMessage::started from request_queued handler instead of using request_started.
That works. Might be better to call it "starting" rather than "started", since it's emitted right before any I/O happens.
Created attachment 297094 [details] [review] Add SoupMessage::starting signal I haven't deprecated anything because request-started is still the only way to get the socket. Once we have soup_message_get_socket() or whatever method, we can deprecate request-started.
Comment on attachment 297094 [details] [review] Add SoupMessage::starting signal > static void >+soup_auth_manager_request_queued (SoupSessionFeature *manager, >+ SoupSession *session, >+ SoupMessage *msg) >+{ >+ g_signal_connect (msg, "starting", >+ G_CALLBACK (auth_msg_starting), manager); Given that you have to do this in basically every SoupSessionFeature, probably we should just have SoupSessionFeature proxy the signal to a new message_started vmethod. Also, the test programs can keep using the deprecated signal for simplicity. (In reply to Carlos Garcia Campos from comment #9) > I haven't deprecated anything because request-started is still the only way > to get the socket. Once we have soup_message_get_socket() or whatever > method, we can deprecate request-started. Yeah, but I don't think anyone except the test programs actually needs the socket param. And it's unclear now why there are two nearly identical signals. So I think we should deprecate request-started now. (Possibly as a second commit.)
(In reply to Dan Winship from comment #10) > Comment on attachment 297094 [details] [review] [review] > Add SoupMessage::starting signal Thanks for the review. > > static void > >+soup_auth_manager_request_queued (SoupSessionFeature *manager, > >+ SoupSession *session, > >+ SoupMessage *msg) > >+{ > >+ g_signal_connect (msg, "starting", > >+ G_CALLBACK (auth_msg_starting), manager); > > Given that you have to do this in basically every SoupSessionFeature, > probably we should just have SoupSessionFeature proxy the signal to a new > message_started vmethod. I'm not sure, most of the features connect to message signals in queued and disconnect them in unqueued, so this is looks very consistent, and we don't want to add a new vmethod for every message signal. > Also, the test programs can keep using the deprecated signal for simplicity. I left the tests using the socket, or very related to the connections with the request_started, but moved some others just as a way to have some test coverage for the ::starting signal without having to add a specific test case for it :-) > (In reply to Carlos Garcia Campos from comment #9) > > I haven't deprecated anything because request-started is still the only way > > to get the socket. Once we have soup_message_get_socket() or whatever > > method, we can deprecate request-started. > > Yeah, but I don't think anyone except the test programs actually needs the > socket param. And it's unclear now why there are two nearly identical > signals. So I think we should deprecate request-started now. (Possibly as a > second commit.) Ok, I never liked deprecating API until there's a replacement, but as you say, it probably doesn't affect anybody expect our tests.
Comment on attachment 297094 [details] [review] Add SoupMessage::starting signal ok then, looks good I guess except: >+void soup_message_starting (SoupMessage *msg); needs to be tagged SOUP_AVAILABLE_IN_2_50, and needs to be added to libsoup-2.4.sym and docs/reference/libsoup-2.4-sections.txt. (It looks like we're not actually documenting the signal-emitting functions, so just add it to the Private section with the rest of them.)
(In reply to Dan Winship from comment #12) > Comment on attachment 297094 [details] [review] [review] > Add SoupMessage::starting signal > > ok then, looks good I guess except: > > >+void soup_message_starting (SoupMessage *msg); > > needs to be tagged SOUP_AVAILABLE_IN_2_50, and needs to be added to > libsoup-2.4.sym and docs/reference/libsoup-2.4-sections.txt. (It looks like > we're not actually documenting the signal-emitting functions, so just add it > to the Private section with the rest of them.) Sure, I thought those macros were only for public API.
Created attachment 297627 [details] [review] Updated patch Deprecated the request-started signal and vmethod in feature interface, and added new symbols to docs and availability macro, etc.
Comment on attachment 297627 [details] [review] Updated patch Pushed.