bytes_read for hijacked connections?

[Mygod]

Is there anyway to add bytes_read to the entry if the connection is hijacked, like in forwardproxy? lengthReader is not exported. 🤔

[mholt]

This could probably be done, but it will take some careful work :)

[Mygod]

I guess I will just use reflection meanwhile. :)

[francislavoie]

I guess we can probably export lengthReader. I don't see why not. The only harm that could be done is some module manipulated Length when it shouldn't but why would a module shoot itself in the foot by doing that lol

[Mygod]

While we are on this, can you also export responseRecorder.size and have a way to extract Conn from hijacked connection? For proxies this is useful since this way io.Copy will be able to use splice and do everything in kernel space. (No more userspace buffers!)

[francislavoie]

Exporting caddyhttp.responseRecorder struct should not be necessary because we already export the caddyhttp.ResponseRecorder interface which has a Size() function.

[Mygod]

Yes but that is read-only. I am trying to add to the counter...

[Mygod]

https://man7.org/linux/man-pages/man2/splice.2.html

Linux syscall splice takes fds directly and returns copied bytes. It is not feasible to use responseRecorder.

[francislavoie]

Yes but that is read-only. I am trying to add to the counter...

Ah, that makes sense. We could maybe add an IncrementSize(int n) method? 🤔

Linux syscall splice takes fds directly and returns copied bytes. It is not feasible to use responseRecorder.

I'm not sure I understand, are you saying "nevermind I don't need it actually" with that?

[Mygod]

Quite the opposite. I was explaining why I needed those. Currently I am using reflection like this.

[francislavoie]

Okay. Would IncrementSize(int n) work for you?

It's awkward, we can't rename caddyhttp.responseRecorder to caddyhttp.ResponseRecorder because it already exists as an interface (name conflict) so it would have to be renamed as something else and I can't think of a name that would work. We also probably shouldn't rename the interface because it's already exported and "part of the API" (even if I doubt many plugins actually use it right now).

[Mygod]

Yes that would work, and I also would like an API for getting the underlying Conn returned from hijacked connection. 😅

[francislavoie]

I'm not sure I understand, your code is what calls Hijack(). It becomes your code's responsibility to handle the Conn.

[Mygod]

That is indeed what my code is doing. I am just wondering if the reflection part I highlighted above can be replaced with stable API. :)

[mholt]

I wonder, should we be counting the bytes written even after a hijack?

[Mygod]

I wonder, should we be counting the bytes written even after a hijack?

It would be a good idea not to for the reasons I specified above (kernel space io.Copy). Exposing a separate interface for logging bytes is preferred?

[francislavoie]

I'm lost at this point. What's the status quo? Do we still need #5807? Is it useful?

[Mygod]

I think #5807 needs also support for updating read bytes...

[francislavoie]

It's exported, you can just increment .Length 🤔

[Mygod]

That's written bytes. I think the API for bytes read is .size. I'm currently doing this reflection as workaround.

[francislavoie]

Ah, so you're asking for responseRecorder as well.

https://github.com/caddyserver/caddy/blob/master/modules%2Fcaddyhttp%2Fresponsewriter.go#L61-L69

@mholt are you ok with exporting that too?

[mholt]

If we do, we'd essentially be replacing the interface with a concrete type:

https://github.com/caddyserver/caddy/blob/master/modules/caddyhttp/responsewriter.go#L266

I don't know anywhere that we use the interface with another type, so that should be fine.

[francislavoie]

@mholt I'm trying to do that now, but I get errors all over, like:

cannot use rr (variable of type ResponseRecorder) as io.Writer value in argument to io.Copy: ResponseRecorder does not implement io.Writer (method Write has pointer receiver) compiler(InvalidIfaceAssign)

I don't quite understand how to do this, apparently it can't be an io.Writer because we're using a pointer receiver, which is necessary to update size while writing 🤔

[francislavoie]

@Mygod at this point I don't think I'll be able to finish this, so it might make more sense if you write the PR for this.

[WeidiDeng]

@Mygod size is already updated when writing and Reading From. The only things left are WriteTo and number of bytes read.

If you really want to use the underlying net.Conn to do splice, checkout caddyhttp.ConnCtxKey. Though I guess you already know you need to Hijack first to use it safely and handling already buffered data.