Creating a non stateless Transport, issue in subtransport_action

[AmarOk1412]

Hi!

First thing first, I'd like to say that this crate is nice and it's pretty cool to use.

Now, I think creating custom transport, like https://github.com/rust-lang/git2-rs/tree/master/git2-curl but with rpc=false will fail. I may be missing a thing, but I'm currently writing a custom transport with this crate. The transport is using a bidirectional channel with a custom & minimal git server I did.

struct WolfTransport {
    channel: Arc<Mutex<Channel<Vec<u8>, Vec<u8>>>>,
}

(...)

Transport::smart(
        remote,
        false, // Here git2-curl is true
        WolfTransport {
            channel
        },
    )

Now, just to get a trace, this is what I see and what is problematic:

Client: git-2-rs:transport.rs => subtransport_action with action = upload-pack-ls
Client: SmartSubtransport::action (with action = upload-pack-ls) Init subtransport, return Ok(Box::new(WolfSubTransport {
Client: WolfSubTransport::read()
Server: recv pkt: 0028git-upload-pack /zdshost=localhost
Server: send references capabilities
006ac29eb686c3638633bff5b852ee1ed76211882ba1 HEADside-band side-band-64k shallow no-progress include-tag
003f7300c6f39366e2c0bcaf99efc05cc45e8511a384 refs/heads/master
0046c29eb686c3638633bff5b852ee1ed76211882ba1 refs/remotes/origin/HEAD
0048c29eb686c3638633bff5b852ee1ed76211882ba1 refs/remotes/origin/master
Client: WolfSubTransport::read()
Server: send flush (0000)
Client: SmartSubtransport::action (with action = upload-pack) Init subtransport, return Ok(Box::new(WolfSubTransport {
Client: WolfSubTransport::write()
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Error { code: -1, klass: 35, message: "unrecoverable internal error: \'t->rpc || t->current_stream == stream\'" }', src/main.rs:33:53

I don't know if it's clear, but the issue is located during the second Init subtransport. libgit2 will call subtransport_action with action=upload-pack and here, subtransport_action should and, because rpc=false re-use previous initialized stream to be able to send want xxxxxxxxx packets and start packfile negotiation.

This is basically what we will have in cpp with libgit2:

int
SubTransportAction(git_smart_subtransport_stream** out,
                      git_smart_subtransport* transport,
                      const char* url,
                      git_smart_service_t action)
{
    auto* sub = reinterpret_cast<SubTransport*>(transport);
    if (!sub || !sub->remote) {
        return -1;
    }

    if (action == GIT_SERVICE_UPLOADPACK_LS) {
        auto stream = std::make_unique<Stream>();
        // Init Stream cb
        sub->stream = std::move(stream);
        *out = &sub->stream->base;
        return 0;
    } else if (action == GIT_SERVICE_UPLOADPACK) {
        if (sub->stream) {
            *out = &sub->stream->base;
            return 0;
        }
        return -1;
    }
    return 0;
}

However in current implementation (

git2-rs/src/transport.rs

Line 250 in 6880563

let obj = match transport.obj.action(url, action) {

) we will always generate a new stream else we will abort when the negotiation will starts (https://github.com/libgit2/libgit2/blob/27e34f9b9843f7bcc33a4ccfe3e395fe303cba63/src/transports/smart.c#L349)

So imho subtransport_action should be rewritten in order to be able to return the previous stream if necessary. I don't have a fix and don't know what would be the preferred solution (or if I missed something, I just saw that this morning at 1am).

Best regards,

[alexcrichton]

Ah this may have been me misunderstanding how libgit2 works in this regard. Feel free to send a PR, though!

[AmarOk1412]

@alexcrichton cool. These days I'm a bit taken, but will try to come with something and will update this issue or send a PR.

Maybe I missed something too, one of the major issue is that the doc for subtransport is not generated https://libgit2.org/libgit2/ and sometimes this needs more digging in their code.

The full code I use is located there: https://github.com/AmarOk1412/p2p-internals-git/tree/main/src

[AmarOk1412]

I did a quick (& dirty) test for my use-case with

diff --git a/src/transport.rs b/src/transport.rs
index 83d7557..2cff0a2 100644
--- a/src/transport.rs
+++ b/src/transport.rs
@@ -87,6 +87,7 @@ struct TransportData {
 #[repr(C)]
 struct RawSmartSubtransport {
     raw: raw::git_smart_subtransport,
+    stream: Option<*mut raw::git_smart_subtransport_stream>,
     obj: Box<dyn SmartSubtransport>,
 }
 
@@ -142,6 +143,7 @@ impl Transport {
                 close: Some(subtransport_close),
                 free: Some(subtransport_free),
             },
+            stream: None,
             obj: Box::new(subtransport),
         });
         let mut defn = raw::git_smart_subtransport_definition {
@@ -246,23 +248,33 @@ extern "C" fn subtransport_action(
             raw::GIT_SERVICE_RECEIVEPACK => Service::ReceivePack,
             n => panic!("unknown action: {}", n),
         };
-        let transport = &mut *(raw_transport as *mut RawSmartSubtransport);
-        let obj = match transport.obj.action(url, action) {
-            Ok(s) => s,
-            Err(e) => {
-                set_err(&e);
-                return e.raw_code() as c_int;
+        let mut transport = &mut *(raw_transport as *mut RawSmartSubtransport);
+        // TODO rpc & match action type
+        match transport.stream {
+            None => {
+                let obj = match transport.obj.action(url, action) {
+                    Ok(s) => s,
+                    Err(e) => {
+                        set_err(&e);
+                        return e.raw_code() as c_int;
+                    }
+                };
+                *stream = mem::transmute(Box::new(RawSmartSubtransportStream {
+                    raw: raw::git_smart_subtransport_stream {
+                        subtransport: raw_transport,
+                        read: Some(stream_read),
+                        write: Some(stream_write),
+                        free: Some(stream_free),
+                    },
+                    obj: obj,
+                }));
+                transport.stream = Some(*stream);
+            },
+            Some(s) => {
+                *stream = s;
             }
         };
-        *stream = mem::transmute(Box::new(RawSmartSubtransportStream {
-            raw: raw::git_smart_subtransport_stream {
-                subtransport: raw_transport,
-                read: Some(stream_read),
-                write: Some(stream_write),
-                free: Some(stream_free),
-            },
-            obj: obj,
-        }));
+
         0
     })
     .unwrap_or(-1)

So basically what I can propose is to add stream: Option<*mut raw::git_smart_subtransport_stream>, in RawSmartSubtransport. And then, if rpc is on & action == uploadpackls|receivepackls, we ask the user of the lib to implement transport.obj.action(url, action), else if rpc is on & action == uploadpack|receivepack set the stream to previous value. If rpc is off, always call transport.obj.action.

Also I looked a bit into libgit2, and they do the thing I describe into this file:
https://github.com/libgit2/libgit2/blob/27e34f9b9843f7bcc33a4ccfe3e395fe303cba63/src/transports/git.c#L232

_git_uploadpack_ls set the stream to previous value.

So, if you agree with this approach, I will do a PR :)