Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Utilize AbortController (Request.signal) for APIs that make outgoing … #187

Closed
wants to merge 18 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,9 @@ To be released.
- Let the `fedify lookup` command take multiple arguments.
[[#173], [#186] by PGD]

- Added options related to `AbortController`.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Fix list indentation.

The indentation of the bullet point doesn't match the style of other entries in the changelog.

Apply this diff to fix the indentation:

- -  Added options related to `AbortController`.
+ - Added options related to `AbortController`.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- Added options related to `AbortController`.
- Added options related to `AbortController`.
🧰 Tools
🪛 Markdownlint (0.35.0)

75-75: Expected: 0; Actual: 1
Unordered list indentation

(MD007, ul-indent)

[[#51] [#187] by PDJ]

Comment on lines +88 to +90
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance the changelog entry for AbortController changes.

The current entry is too brief. Consider expanding it to match the detail level of other entries by:

  1. Listing the specific interfaces that received the signal parameter (e.g., GetDocumentLoaderOptions, GetActorHandleOptions, etc.)
  2. Explaining that this feature enables request timeout control
  3. Following the established format of other entries with proper indentation

Apply this diff to enhance the changelog entry:

- -  Added options related to `AbortController`.
-    [[#51] [#187] by PDJ]
+ -  Added support for request cancellation using `AbortController`.
+    [[#51], [#187] by PDJ]
+
+     -  Added optional `signal` parameter to the following interfaces:
+         -  `GetDocumentLoaderOptions`
+         -  `GetActorHandleOptions`
+         -  `LookupObjectOptions`
+         -  `LookupWebFingerOptions`
+
+     -  The `signal` parameter enables control over the maximum duration
+        of outgoing API requests, preventing them from hanging indefinitely.

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 Markdownlint (0.35.0)

88-88: Expected: 0; Actual: 1
Unordered list indentation

(MD007, ul-indent)


[SvelteKit]: https://kit.svelte.dev/
[#162]: https://github.com/dahlia/fedify/issues/162
Expand Down
22 changes: 22 additions & 0 deletions docs/manual/opentelemetry.md
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,11 @@ spans:
| `object_integrity_proofs.verify` | Internal | Verifies the object integrity proof. |
| `webfinger.handle` | Server | Handles the WebFinger request. |
| `webfinger.lookup` | Client | Looks up the WebFinger resource. |
| Operation | Span type | Description |
|----------------------|-----------|-----------------------------------|
| `Federation.fetch()` | Server | Serves the incoming HTTP request. |
| `lookupWebFinger()` | Client | Looks up the WebFinger resource. |
| `handleWebFinger()` | Server | Handles the WebFinger request. |

More operations will be instrumented in the future releases.

Expand Down Expand Up @@ -172,4 +177,21 @@ for ActivityPub:
| `webfinger.resource.scheme` | string | The scheme of the queried resource URI. | `"acct"` |

[attributes]: https://opentelemetry.io/docs/specs/otel/common/#attribute
| Attribute | Type | Description | Example |
|----------------------------------|----------|---------------------------------------------------------------------------------|----------------------------------------------------|
| `activitypub.activity.id` | string | The URI of the activity object. | `"https://example.com/activity/1"` |
| `activitypub.activity.type` | string[] | The qualified URI(s) of the activity type(s). | `["https://www.w3.org/ns/activitystreams#Create"]` |
| `activitypub.activity.to` | string[] | The URI(s) of the recipient collections/actors of the activity. | `["https://example.com/1/followers/2"]` |
| `activitypub.activity.cc` | string[] | The URI(s) of the carbon-copied recipient collections/actors of the activity. | `["https://www.w3.org/ns/activitystreams#Public"]` |
| `activitypub.activity.retries` | int | The ordinal number of activity resending attempt (if and only if it's retried). | `3` |
| `activitypub.actor.id` | string | The URI of the actor object. | `"https://example.com/actor/1"` |
| `activitypub.actor.type` | string[] | The qualified URI(s) of the actor type(s). | `["https://www.w3.org/ns/activitystreams#Person"]` |
| `activitypub.object.id` | string | The URI of the object or the object enclosed by the activity. | `"https://example.com/object/1"` |
| `activitypub.object.type` | string[] | The qualified URI(s) of the object type(s). | `["https://www.w3.org/ns/activitystreams#Note"]` |
| `activitypub.object.in_reply_to` | string[] | The URI(s) of the original object to which the object reply. | `["https://example.com/object/1"]` |
| `activitypub.inboxes` | int | The number of inboxes the activity is sent to. | `12` |
| `activitypub.shared_inbox` | boolean | Whether the activity is sent to the shared inbox. | `true` |
| `webfinger.resource` | string | The queried resource URI. | `"acct:[email protected]"` |
| `webfinger.resource.scheme` | string | The scheme of the queried resource URI. | `"acct"` |

[OpenTelemetry Semantic Conventions]: https://opentelemetry.io/docs/specs/semconv/
8 changes: 7 additions & 1 deletion src/runtime/docloader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,11 @@ export interface GetDocumentLoaderOptions {
* Whether to preload the frequently used contexts.
*/
skipPreloadedContexts?: boolean;

/**
* An optional abort signal to cancel the request.
*/
signal?: AbortSignal | null;
}

/**
Expand All @@ -255,7 +260,7 @@ export interface GetDocumentLoaderOptions {
* @since 1.3.0
*/
export function getDocumentLoader(
{ allowPrivateAddress, skipPreloadedContexts, userAgent }:
{ allowPrivateAddress, skipPreloadedContexts, userAgent, signal }:
GetDocumentLoaderOptions = {},
): DocumentLoader {
async function load(url: string): Promise<RemoteDocument> {
Expand Down Expand Up @@ -284,6 +289,7 @@ export function getDocumentLoader(
// to work around it we specify `redirect: "manual"` here too:
// https://github.com/oven-sh/bun/issues/10754
redirect: "manual",
signal,
});
// Follow redirects manually to get the final URL:
if (
Expand Down
33 changes: 28 additions & 5 deletions src/vocab/actor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,12 @@ export interface GetActorHandleOptions extends NormalizeActorHandleOptions {
* @since 1.3.0
*/
tracerProvider?: TracerProvider;

/**
* An optional abort signal to cancel the request.
* @since 1.3.0
*/
signal?: AbortSignal | null;
}

/**
Expand Down Expand Up @@ -166,10 +172,21 @@ async function getActorHandleInternal(
): Promise<`@${string}@${string}` | `${string}@${string}`> {
const actorId = actor instanceof URL ? actor : actor.id;
if (actorId != null) {
const result = await lookupWebFinger(actorId, {
userAgent: options.userAgent,
tracerProvider: options.tracerProvider,
});
let result;
try {
result = await lookupWebFinger(actorId, {
userAgent: options.userAgent,
tracerProvider: options.tracerProvider,
signal: options.signal,
});
} catch (error) {
if ((error as Error).name === "AbortError") {
throw new Error("Actor handle lookup was aborted", { cause: error });
}

throw error;
}

if (result != null) {
const aliases = [...(result.aliases ?? [])];
if (result.subject != null) aliases.unshift(result.subject);
Expand All @@ -184,6 +201,7 @@ async function getActorHandleInternal(
alias,
options.userAgent,
options.tracerProvider,
options.signal,
)
) {
continue;
Expand Down Expand Up @@ -212,8 +230,13 @@ async function verifyCrossOriginActorHandle(
alias: string,
userAgent: GetUserAgentOptions | string | undefined,
tracerProvider: TracerProvider | undefined,
signal?: AbortSignal | null,
): Promise<boolean> {
const response = await lookupWebFinger(alias, { userAgent, tracerProvider });
const response = await lookupWebFinger(alias, {
userAgent,
tracerProvider,
signal,
});
if (response == null) return false;
for (const alias of response.aliases ?? []) {
if (new URL(alias).href === actorId) return true;
Expand Down
7 changes: 7 additions & 0 deletions src/vocab/lookup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,12 @@ export interface LookupObjectOptions {
* @since 1.3.0
*/
tracerProvider?: TracerProvider;

/**
* An optional abort signal to cancel the request.
* @since 1.3.0
*/
signal?: AbortSignal | null;
}

const handleRegexp =
Expand Down Expand Up @@ -147,6 +153,7 @@ async function lookupObjectInternal(
const jrd = await lookupWebFinger(identifier, {
userAgent: options.userAgent,
tracerProvider: options.tracerProvider,
signal: options.signal,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Propagate abort signal to documentLoader calls.

While the signal is correctly passed to lookupWebFinger, it should also be propagated to the documentLoader calls to ensure consistent cancellation behavior throughout the lookup process.

Consider updating the documentLoader calls:

  if (identifier.protocol === "http:" || identifier.protocol === "https:") {
    try {
-     const remoteDoc = await documentLoader(identifier.href);
+     const remoteDoc = await documentLoader(identifier.href, { signal: options.signal });
      document = remoteDoc.document;
    } catch (error) {
      logger.debug("Failed to fetch remote document:\n{error}", { error });
    }
  }

And:

    try {
-     const remoteDoc = await documentLoader(l.href);
+     const remoteDoc = await documentLoader(l.href, { signal: options.signal });
      document = remoteDoc.document;
      break;
    } catch (error) {

Committable suggestion skipped: line range outside the PR's diff.

});
if (jrd?.links == null) return null;
for (const l of jrd.links) {
Expand Down
6 changes: 6 additions & 0 deletions src/webfinger/lookup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@ export interface LookupWebFingerOptions {
* is used.
*/
tracerProvider?: TracerProvider;

/**
* An optional abort signal to cancel the request.
*/
signal?: AbortSignal | null;
}

/**
Expand Down Expand Up @@ -107,6 +112,7 @@ async function lookupWebFingerInternal(
let response: Response;
try {
response = await fetch(url, {
signal: options?.signal,
headers: {
Accept: "application/jrd+json",
"User-Agent": typeof options.userAgent === "string"
Expand Down