GNOME Bugzilla – Bug 744788
The life cycle of cached resources message is inconsistent with normal messages
Last modified: 2015-03-02 08:48:43 UTC
There are two problems here: 1- We are not kicking the queue when marking a cached resource message as FINISHING, so the message is never unqueued unless there's another request that kicks the queue. 2- We are marking the messages as FINISHING when sending the response, instead of when io has completed, so the message can be unqueued while the client is still reading the stream.
Created attachment 297274 [details] [review] kick the message queue when a cached resource finishes
Created attachment 297275 [details] [review] Do not unqueue messages of cached resources until finished
Comment on attachment 297274 [details] [review] kick the message queue when a cached resource finishes >+ session = item->session; > async_send_request_return_result (item, g_object_ref (stream), NULL); >+ soup_session_kick_queue (session); Is the separate "session" variable because item might get freed by async_send_request_return_result()? Because if so, then it might also be possible for the session itself to be freed (if the caller unrefs the session from inside the callback), in which case we'd want to take an extra ref on session here. (It's also possible that that can't happen because it's guaranteed that something else (a GTask or whatever) will always be holding a ref on the session at this point... I'm not sure.) Other than that possibility, this looks right.
(In reply to Dan Winship from comment #3) > Comment on attachment 297274 [details] [review] [review] > kick the message queue when a cached resource finishes > > >+ session = item->session; > > async_send_request_return_result (item, g_object_ref (stream), NULL); > >+ soup_session_kick_queue (session); > > Is the separate "session" variable because item might get freed by > async_send_request_return_result()? Yes. > Because if so, then it might also be > possible for the session itself to be freed (if the caller unrefs the > session from inside the callback), in which case we'd want to take an extra > ref on session here. Yes, I guess it's harmaless to take a ref and it will be safer. > (It's also possible that that can't happen because it's guaranteed that > something else (a GTask or whatever) will always be holding a ref on the > session at this point... I'm not sure.) I'll check anyway, if in doubt, I'll take a ref. > Other than that possibility, this looks right. Thanks.
(In reply to Carlos Garcia Campos from comment #4) > (In reply to Dan Winship from comment #3) > > Comment on attachment 297274 [details] [review] [review] [review] > > kick the message queue when a cached resource finishes > > > > >+ session = item->session; > > > async_send_request_return_result (item, g_object_ref (stream), NULL); > > >+ soup_session_kick_queue (session); > > > > Is the separate "session" variable because item might get freed by > > async_send_request_return_result()? > > Yes. > > > Because if so, then it might also be > > possible for the session itself to be freed (if the caller unrefs the > > session from inside the callback), in which case we'd want to take an extra > > ref on session here. > > Yes, I guess it's harmaless to take a ref and it will be safer. > > > (It's also possible that that can't happen because it's guaranteed that > > something else (a GTask or whatever) will always be holding a ref on the > > session at this point... I'm not sure.) > > I'll check anyway, if in doubt, I'll take a ref. > > > Other than that possibility, this looks right. > > Thanks. Ok, this is what happens here. async_return_from_cache() can be called from idle_return_from_cache_cb, scheduled when the resources is FRESH, or from conditional_get_ready_cb, when the conditional request finishes if the resources needs validation. In this case, the conditional request finishes and message_completed callback is called, this kicks the queue, and conditional_get_ready_cb is also called that calls async_return_from_cache. This does the following (for the original message): 1.- sets item state to FINISHING 2.- calls async_send_request_return_result 3.- kicks the queue When the task is completed in step 2, the async run queue is processed (because of the previous kick_queue in message_completed callback of the conditional request), and since the original message was queued before the conditional one, and the state was switched to FINISHING in state 1, the item is processed and finished. In step 3, the item has already been unqueued. This doesn't happen when async_return_from_cache is called because the resource is FRESH. So, I think the right solution here is moving the step 1 after 2.
Created attachment 298255 [details] [review] kick the message queue when a cached resource finishes
(In reply to Carlos Garcia Campos from comment #6) > Created attachment 298255 [details] [review] [review] > kick the message queue when a cached resource finishes I've just realized that this was changed in the next patch anyway, to avoid finishing the message before the io has completed. But still, it's better to land the right patch just in case we end up reverting the second one for whatever reason at some point.
Comment on attachment 298255 [details] [review] kick the message queue when a cached resource finishes Pushed.
Comment on attachment 297275 [details] [review] Do not unqueue messages of cached resources until finished Pushed.