Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Commit

Permalink
Check native sliding sync support against an unstable feature flag
Browse files Browse the repository at this point in the history
The `OPTIONS` approach from #12492 doesn't work because Synapse *always* responds with 204 (success) to `OPTIONS` requests, as described here: element-hq/synapse#17153

We further can't use `HEAD` because it's not part of the allowed CORS methods, meaning the browser will mask the exact status code and error message from us, and the proxy hangs on the request anyways: matrix-org/sliding-sync#429

To avoid these problems, we instead search for an unstable feature flag to be exposed by the server. Presence of this flag denotes native support. See https://github.com/matrix-org/matrix-spec-proposals/pull/3575/files#r1588877046 for details.

Implementations which support sliding sync natively will need to update to support this new unstable feature flag usage.
  • Loading branch information
turt2live committed May 3, 2024
1 parent 9684dd5 commit 7470ed7
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 48 deletions.
23 changes: 7 additions & 16 deletions src/SlidingSyncManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -372,26 +372,17 @@ export class SlidingSyncManager {
}

/**
* Check if the server "natively" supports sliding sync (at the unstable endpoint).
* Check if the server "natively" supports sliding sync (with an unstable endpoint).
* @param client The MatrixClient to use
* @return Whether the "native" (unstable) endpoint is up
* @return Whether the "native" (unstable) endpoint is supported
*/
public async nativeSlidingSyncSupport(client: MatrixClient): Promise<boolean> {
try {
// We use OPTIONS to avoid causing a real sync to happen, as that may be intensive or encourage
// middleware software to start polling as our access token (thus stealing our to-device messages).
// See https://github.com/element-hq/element-web/issues/27426
// XXX: Using client.http is a bad thing - it's meant to be private access. See `client.http` for details.
await client.http.authedRequest<void>(Method.Options, "/sync", undefined, undefined, {
localTimeoutMs: 10 * 1000, // 10s
prefix: "/_matrix/client/unstable/org.matrix.msc3575",
});
} catch (e) {
return false; // 404, M_UNRECOGNIZED
// Per https://github.com/matrix-org/matrix-spec-proposals/pull/3575/files#r1589542561
const support = await client.doesServerSupportUnstableFeature("org.matrix.msc3575");
if (support) {
logger.log("nativeSlidingSyncSupport: sliding sync advertised as unstable");
}

logger.log("nativeSlidingSyncSupport: sliding sync endpoint is up");
return true; // 200, OK
return support;
}

/**
Expand Down
38 changes: 6 additions & 32 deletions test/SlidingSyncManager-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,7 @@ limitations under the License.

import { SlidingSync } from "matrix-js-sdk/src/sliding-sync";
import { mocked } from "jest-mock";
import { IRequestOpts, MatrixClient, MatrixEvent, Method, Room } from "matrix-js-sdk/src/matrix";
import { QueryDict } from "matrix-js-sdk/src/utils";
import { MatrixClient, MatrixEvent, Room } from "matrix-js-sdk/src/matrix";

import { SlidingSyncManager } from "../src/SlidingSyncManager";
import { stubClient } from "./test-utils";
Expand Down Expand Up @@ -261,41 +260,16 @@ describe("SlidingSyncManager", () => {
it("should make an OPTIONS request to avoid unintended side effects", async () => {
// See https://github.com/element-hq/element-web/issues/27426

// Developer note: We mock this in a truly terrible way because of how the call is done. There's not
// really much we can do to avoid it.
client.http = {
async authedRequest(
method: Method,
path: string,
queryParams?: QueryDict,
body?: Body,
paramOpts: IRequestOpts & {
doNotAttemptTokenRefresh?: boolean;
} = {},
): Promise<any> {
// XXX: Ideally we'd use ResponseType<> like in the real thing, but it's not exported
expect(method).toBe(Method.Options);
expect(path).toBe("/sync");
expect(queryParams).toBeUndefined();
expect(body).toBeUndefined();
expect(paramOpts).toEqual({
localTimeoutMs: 10 * 1000, // 10s
prefix: "/_matrix/client/unstable/org.matrix.msc3575",
});
return {};
},
} as any;

const unstableSpy = jest.spyOn(client, "doesServerSupportUnstableFeature").mockImplementation(async (feature: string) => {
expect(feature).toBe("org.matrix.msc3575");
return true;
});
const proxySpy = jest.spyOn(manager, "getProxyFromWellKnown").mockResolvedValue("proxy");

expect(SlidingSyncController.serverSupportsSlidingSync).toBeFalsy();

await manager.checkSupport(client); // first thing it does is call nativeSlidingSyncSupport

// Note: if this expectation is failing, it may mean the authedRequest mock threw an expectation failure
// which got consumed by `nativeSlidingSyncSupport`. Log your errors to discover more.
expect(proxySpy).not.toHaveBeenCalled();

expect(unstableSpy).toHaveBeenCalled();
expect(SlidingSyncController.serverSupportsSlidingSync).toBeTruthy();
});
});
Expand Down

0 comments on commit 7470ed7

Please sign in to comment.