drasticactions / FishyFlip

Fishyflip - a .NET ATProtocol/Bluesky Library
MIT License
62 stars 7 forks source link

HttpClientHandler Automatic Decompression #57

Closed themayarose closed 1 month ago

themayarose commented 1 month ago

Hi! Since a few days ago, BlueSky seemingly started sending compressed responses to endpoints' calls. However, in ATProtocolOptions, both in branches develop and 1.8, the HttpClient/Handler for the ATProtocol object is repeatedly recreated - and, since System.Http.Net 4.3.2, the default value for AutomaticDecompression is DecompressionMethods.None, resulting in an System.Text.Json exception when it tries to retrieve the responses.

I have made a very ugly workaround, that I call before each endpoint call, to override the ATProtocol's client's handler. However, it would be nice if this were baked into FishyFlip.

public ATProtocol Protocol { get; }

private void FixHttpClient() {
        var t = Protocol.Options.HttpClient.GetType();

        FieldInfo? field = null;

        while (field is null && t is not null) {
            field = t.GetField("_handler", BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance);
            t = t.BaseType;
        }

        if (field is null) return;

        var newHandler = new HttpClientHandler() {
            AutomaticDecompression = DecompressionMethods.GZip | DecompressionMethods.Deflate,
            MaxRequestContentBufferSize = int.MaxValue
        };

        field.SetValue(Protocol.Options.HttpClient, newHandler);
    }
drasticactions commented 1 month ago

Was it a specific endpoint or server that's failing, or all servers failing for you? And which framework are you using? .NET 8? Framework?

I ran my unit tests and they all passed for unauthed and authed sessions, but it only uses one PDS server, and it did not have compression on it. So I don't know if that means some instances operate differently or not. I'm happy to change it (it doesn't really affect performance AFAIK, and it's in one place) but I'm interested.

As for the HttpClientHandler: https://github.com/drasticactions/FishyFlip/blob/develop/src/FishyFlip/ATProtocolOptions.cs#L89

It should be created twice per session: When you start a login session, and when the initial callback happens, since the base address has to change to match the users PDS, which is given upon the initial setting of session. If it's happening repeatedly then that's an issue I need to look at.

themayarose commented 1 month ago

I only tested on the https://bsky.social server (which is returning compressed responses after login), and I didn't know the handler was created only twice, I only assumed it was being recreated because the one I used with the .WithHttpClient method from ATProtocolBuilder didn't make it to after login. Also, I'm using .NET 9 RC1 currently.

I'm working on a bsky client as a personal project, while learning the AT Protocol and your library in the process. That's why I made wrong assumptions earlier, sorry about that 😅

drasticactions commented 1 month ago

I removed that method from the current development branch. It was initially there to let you override the HttpClient but when needing to switch out the PDS URL (the https://bsky.social URI, or whichever instance URL you use by default, Is only used if you don't log in. When you do, it needs to switch to the PDS instance from the user in order to access their information.

https://www.nuget.org/packages/FishyFlip/2.0.0-alpha.45

Every time I access https://bsky.social It doesn't have compression on it, but it could be a CDN thing or maybe something on my computer. Can you please try the alpha version above to see if that works for you?

If others are hitting this or have a PDS or another service URI sending those headers, please respond here so I can test it. Thanks!

themayarose commented 1 month ago

So, I did some digging, and this is what I got. My PDS is https://oyster.us-east.host.bsky.network/. And I can confirm that, without the HttpClientHandler fix for compression, it's still throwing errors. This is what I get when I run it without fixing it:

exception

The first time I got this error was on Tuesday afternoon BRT, but it took me two days to find out the reason, so that's why I only posted the issue yesterday.

Finally, the version 2.0.0-alpha.45 fixes this problem, but it's not setting Protocol.Session, so any requests that need authentication fail with the error Error: ATError: 401 AuthMissing Authentication Required.

(Btw, sorry to reopen the issue, just trying to clarify what happened.)

EDIT: The reason it's not setting Protocol.Session seems to be here - the SetSession call is inside the condition if (session is not null), so it's never called :)

drasticactions commented 1 month ago

@themayarose You don't need to apologize, I asked you first to make sure it works and if it didn't, I would have reopened the issue. It closed because the PR closed, but I should change that going forward.

For the auth stuff, that's because it changes how Authentication works, it's a breaking change that I both need to document when 2.0 comes out and I should have told you about, sorry! The original methods and docs won't work anymore because it doesn't automatically set it up by just calling the endpoint.

In order to support OAuth, I needed to split the original authentication patterns. To use password support, you need to use PasswordSessionManager,

It's on ATProtocol,

https://github.com/drasticactions/FishyFlip/blob/85d0581617b3f77f87385042163121dae6d31a5d/src/FishyFlip/ATProtocol.cs#L172

If you call AuthenticateWithPasswordAsync, with your username and password, you'll get an authenticated session and all user calls should work correctly again. For OAuth, I wrote a sample app for getting tokens here which uses this library for creating it (So, how you would use it in a normal session) but OAuth is still being worked on so I would stick to passwords for now.

When 2.0 comes out I'll have full docs for how to use it.

e. I've created a new issue for 2.0 changes in case anyone else hits it.

https://github.com/drasticactions/FishyFlip/issues/59