search

shadowsocks / go-shadowsocks2

Modern Shadowsocks in Go
Apache License 2.0
4.45k stars 1.39k forks source link

Initialize saltfilter on demand #189

Closed fortuna closed 3 years ago

fortuna commented 3 years ago

This fixes the problem of allocating a lot of unnecessary memory on clients.

fortuna commented 3 years ago

cc: @alalama @bemasc

fortuna commented 3 years ago

@riobard, would you mind reviewing this?

riobard commented 3 years ago

@fortuna Could you please isolate the singleton use case to the internal package for now? I don't feel comfortable with exporting its implementation details. Ideally all call sites of internal.Add and internal.Test should stay untouched, and you can simply change internal.init to internal.InitFilter and manually initialize the filter somehow.

Maybe we should dump all ENV VARs and make them command line flags instead? Sorry I'm on a road trip right now, please correct me if there's any misunderstanding.

And thanks a lot for this PR! :)

fortuna commented 3 years ago

I've restored the original internal interface and reverted all call sites. Does that address your concern?

I didn't change to manual initialization because that may break callers. It's still on-demand.

fortuna commented 3 years ago

My view on flags and ENV: those should all live in binaries, not libraries. Libraries should provide ways to inject parameters, so that each caller can tweak to their needs. For example, in outline-ss-server, we inject a salt generator: https://github.com/Jigsaw-Code/outline-ss-server/blob/ac58257355e60b06f8d00d22128a7dd85cbde3e1/shadowsocks/stream.go#L45 This way client and server can behave differently. We actually need that, since the server uses signed salts.

riobard commented 3 years ago

Let's make the change for now, and I'll think about how to properly fix it once I finish the road trip.

fortuna commented 3 years ago

To clarify a point made at https://github.com/shadowsocks/go-shadowsocks2/pull/165#discussion_r497954942, the filter is an issue not only for using the code as a library, but also for the go-shadowsocks2 client. Having replay protection on the client is overkill.