Created attachment 228247 [details] [review]
WIP port to SoupRequest

*** Bug 687758 has been marked as a duplicate of this bug. ***

I'll add --disable-http to the jhbuild moduleset since it doesn't build at the moment with or without this patch.

Created attachment 230792 [details] [review]
http: Port to SoupRequester

The original patch was on a good way, thanks for this, yet needed several tweaks. I've added SoupRequester initialization and feature registration to be able to actually use it.

Also came across weird assertion failure:

> (process:29393): libsoup-CRITICAL **: soup_session_send_request_async: assertion `use_thread_context' failed

so I had to set the "use-thread-context" property on async SoupSession to TRUE. Which I don't think it's necessarily bad thing, the code looks to be well thread-aware.

The DAV backend part needed almost no changes. I've also kept SoupOutputStream around since SoupRequest (SoupRequestHTTP) seems to be only for reading, without ability to actually push some data and open a GOutputStream channel. There are no extern library imports like soup-input-stream.c used to have, we should be on a safe side at least for the moment.

Hm... I thought I was cc:ed on this bug but I guess not.

Anyway, I had also started working on this, but never attached the patches because I hadn't tested them yet. My version is more complicated than Tomas's, because I tried to preserve the fact that SoupInputStream was seekable, while the stream returned from SoupRequestHTTP isn't.

This patch is actually a bit out of date, since as of a few days ago, SoupRequester is now deprecated and you can just do "soup_session_request()" instead of "soup_requester_request()"...

Created attachment 231336 [details] [review]
http: replace SoupInputStream with SoupRequest

Replace the hacky SoupInputStream with a new GVfsHttpInputStream that
is a wrapper around SoupRequest. (We need a wrapper stream rather than
just using SoupRequest directly because we want the stream here to be
seekable, which requires cancelling and re-sending the HTTP request
and getting a new underlying stream.)

The http and dav backends still use both a sync and an async
SoupSession, even though this is no longer necessary, since changing
this would require a lot of rewriting of code that currently works.

Created attachment 231337 [details] [review]
http: Simplify job failure handling

gvfsbackendhttp defined g_vfs_job_failed_from_http_status(), but
didn't export this, so gvfsbackenddav was sort of forced to
reimplement it. Fix that by exporting it as http_job_failed().

Created attachment 231338 [details] [review]
dav: kill SoupOutputStream

SoupOutputStream was never particularly useful, since we ended up not
doing chunked requests (since server support for them is mostly
nonexistent). So kill SoupOutputStream off and just use a
GMemoryOutputStream instead.

(In reply to comment #8)
> Created an attachment (id=231338) [details] [review]
> dav: kill SoupOutputStream
> 
> SoupOutputStream was never particularly useful, since we ended up not
> doing chunked requests (since server support for them is mostly
> nonexistent). So kill SoupOutputStream off and just use a
> GMemoryOutputStream instead.

I have never paid much attention to our dav backend, but looking at the memory stream and data passing to SoupMessage, it seems to me like the old known problem of high memory usage (equal to the full transferred file size) still stands. Even if we had a way to push data to SoupMessage by blocks (i.e. callbacks supplying data), I guess we still fail to know the overall data size to be transferred at the beginning, right? Just thinking how to work around that (if the chunked requests are not an option)...

Writing to dav doesn't work:

(process:22013): GLib-GIO-CRITICAL **: g_memory_output_stream_steal_data: assertion `g_output_stream_is_closed (G_OUTPUT_STREAM (ostream))' failed

(process:22013): libsoup-CRITICAL **: soup_message_set_request: assertion `content_type != NULL || req_length == 0' failed

and no error is returned to client either (operation still waiting for something).


And trying to read from http makes the backend segfault trying to print assertion failure in 

+ Trace 231292

117)       priv->req = soup_requester_request_uri (priv->requester, priv->uri, &error);
118)       g_assert_no_error (error);


Looking into it.

Review of attachment 231336 [details] [review]:

Also, for some reason I'm getting the same assert like last time:

> (process:3287): libsoup-CRITICAL **: soup_session_send_request_async: assertion `use_thread_context' failed

Adding

> g_object_set (G_OBJECT (backend->session_async), "use-thread-context", TRUE, NULL);

in g_vfs_backend_http_init() would make it work. What is the deal behind this property?

::: daemon/gvfshttpinputstream.c
@@ +113,3 @@
+  if (!priv->req)
+    {
+      GError *error;

should be initialized NULL

Review of attachment 231336 [details] [review]:

This last bit makes async file read working again, however for the third read attempt I'm getting 
> soup_requester_request_uri: assertion `SOUP_IS_REQUESTER (requester)' failed
failure which I cannot explain. Could that be related to the "use-thread-context" property I mentioned last time?

::: daemon/gvfshttpinputstream.c
@@ +214,3 @@
+  g_input_stream_clear_pending (http_stream);
+
+  if (soup_request_send_finish (SOUP_REQUEST (object), result, &error))

soup_request_send_finish() returns a GInputStream which should be stored in priv->stream (from the http_stream) pointer.

(In reply to comment #9)
> Even if we had a way to push data to SoupMessage by blocks (i.e.
> callbacks supplying data)

(There will eventually be "soup_request_send_with_body()", which will return a GIOStream so you can first write out the request body, and then read back the response body.)

> I guess we still fail to know the overall data size
> to be transferred at the beginning, right?

Exactly. Maybe we should add some gio API to let the caller (optionally) specify in advance how many bytes they're going to write? Especially if there are other backends it would help too...


(In reply to comment #10)
> Writing to dav doesn't work:

> And trying to read from http makes the backend segfault

yes, as mentioned before, the patch is totally untested


(In reply to comment #11)
> Also, for some reason I'm getting the same assert like last time:
> 
> > (process:3287): libsoup-CRITICAL **: soup_session_send_request_async: assertion `use_thread_context' failed
> 
> Adding
> 
> > g_object_set (G_OBJECT (backend->session_async), "use-thread-context", TRUE, NULL);
> 
> in g_vfs_backend_http_init() would make it work. What is the deal behind this
> property?

It makes SoupSession be gio-like with respect to GMainContexts (using g_main_context_get_thread_default() rather than the old behavior of using a single user-specified GMainContext). Parts of SoupRequester depend on that behavior, so you have to set it if you're going to use SoupRequester.

(In reply to comment #12)
> Review of attachment 231336 [details] [review]:
> 
> This last bit makes async file read working again, however for the third read
> attempt I'm getting 
> > soup_requester_request_uri: assertion `SOUP_IS_REQUESTER (requester)' failed
> failure which I cannot explain.

Uh... maybe a refcounting bug causing the requester to get freed when it's supposed to still be in use?

Review of attachment 231338 [details] [review]:

These two bits make dav uploading working again.

::: daemon/gvfsbackenddav.c
@@ +2511,3 @@
+
+  length = g_memory_output_stream_get_data_size (G_MEMORY_OUTPUT_STREAM (stream));
+  data = g_memory_output_stream_steal_data (G_MEMORY_OUTPUT_STREAM (stream));

Need to explicitly close the memory stream before stealing its data.

@@ +2514,3 @@
+  g_object_unref (stream);
+
+  soup_message_set_request (msg, NULL,

The content_type argument is required to be set if passing data in. Using "application/octet-stream" makes it work, though I'm not sure whether it's accepted by all dav servers.

(In reply to comment #13)
> (There will eventually be "soup_request_send_with_body()", which will return a
> GIOStream so you can first write out the request body, and then read back the
> response body.)

This would be great to have, see the next paragraph.
 
> > I guess we still fail to know the overall data size
> > to be transferred at the beginning, right?
> 
> Exactly. Maybe we should add some gio API to let the caller (optionally)
> specify in advance how many bytes they're going to write? Especially if there
> are other backends it would help too...

It may help mtp and perhaps even obex backends too.

A GIO API would be nice, although in most cases we would be just fine with libsoup changes only. Given that most write operations are actually file copy ops, gvfs internally knows how many data is being transferred (i.e. for progress bars). If soup can provide a GOutputStream or callback where we can put data blocks, preceded by completing the header knowing total data size, and libsoup would gradually be sending data out keeping the connection open while waiting for more data, we would get rid of large memory consumption at all.

New GIO API (on public GOutputStream) would only help some client applications that are saving buffers from memory and know complete file size, such as gedit.

> (In reply to comment #12)
> Uh... maybe a refcounting bug causing the requester to get freed when it's
> supposed to still be in use?

Right, priv->requester in g_vfs_http_input_stream_new() should explicitly take a reference since soup_session_get_feature() owns the returned object. It was not clear on a first sight. We were unreffing it gradually in g_vfs_http_input_stream_finalize() leading to troubles.

---

With all the findings above it seems stable and passed my testing both for http and dav. Could you please post fixed patches or should I post separate patches on top of yours?

(In reply to comment #15)
> With all the findings above it seems stable and passed my testing both for http
> and dav. Could you please post fixed patches or should I post separate patches
> on top of yours?

Feel free to just squash your patches into mine and commit them with whatever authorship / commit message you want.

I've corrected small mistakes in patch files directly where possible and pushed the rest fixes as separate commits. Pushed to master.

Thanks for the patches and please think about my needs in comment 15.

(In reply to comment #17)
> Thanks for the patches and please think about my needs in comment 15.

It's possible (but ugly) to stream the request body now, using the wrote-chunk signal to know when to append the next chunk. (tests/streaming-test.c in the libsoup sources shows how to do it from the server side; the client side is similar.) You just need to call soup_message_headers_set_content_length() beforehand so libsoup can write the correct Content-Length header.

The SoupRequest-based API for request bodies might show up for Christmas, but it's more likely that it won't make 3.8.

Our product implements WebDAV support and for MIME type detection, it takes Content-Type attribute first, then file extension. Since Ubuntu 13.04 (gvfs 1.16.1), all files uploaded via Nautilus to our server have Content-Type of application/octet-stream due to this commit https://git.gnome.org/browse/gvfs/commit/daemon/gvfsbackenddav.c?id=635c3b507e9082d8f8016f18cc14445a7493e456.

As work around, we use file extension to get MIME type in case Content-Type is application/octet-stream. Do you think that we should apply that work around or ask for another solution for this ticket?

(In reply to comment #19)
> https://git.gnome.org/browse/gvfs/commit/daemon/gvfsbackenddav.c?id=635c3b507e9082d8f8016f18cc14445a7493e456.

> The content_type argument is required to be set for soup_message_set_request() > if passing data in. Using "application/octet-stream" makes it work,

The alternative would be to just not use soup_message_set_request(); gvfs could use soup_message_body_append(msg->request_body, ...) instead.

Created attachment 294601 [details] [review]
do not set application octet stream mime type

gvfs-info shows me correct content-type after writing, not octet-stream. Maybe it depends on server configuration, but could you test this patch if it helps (it do what Dan suggested)?

Review of attachment 294601 [details] [review]:

Of the webdav clients I tried, most didn't set this header so it appears that it is not needed. And the code is shorter :-)

Thanks for testing...

Comment on attachment 294601 [details] [review]
do not set application octet stream mime type

commit bccf6a229448ed944963095b673adc5f13649b12