jason810496 commented on code in PR #69302:
URL: https://github.com/apache/airflow/pull/69302#discussion_r3568286598
##########
ts-sdk/src/coordinator/log-channel.ts:
##########
@@ -47,31 +47,45 @@ export interface LogRecord {
const DEFAULT_LOGGER_NAME = "ts-sdk";
+interface LogChannelState {
+ sock: Socket;
+ connected: boolean;
+ closed: boolean;
+}
+
Review Comment:
I think we can keep the logging buffer part. Perhaps in the follow-up, we
can keep the scope as-is for this PR to only focus on the redirecting the logs
to stderr once receive any fatal error.
More specific, the follow-up will target the logging that TS runtime
produces before the socket connects as buffer then send to the log socket once
both side connect.
##########
ts-sdk/src/coordinator/log-channel.ts:
##########
@@ -47,31 +47,45 @@ export interface LogRecord {
const DEFAULT_LOGGER_NAME = "ts-sdk";
+interface LogChannelState {
+ sock: Socket;
+ connected: boolean;
+ closed: boolean;
+}
+
export class LogChannel {
- private readonly sock: Socket;
+ private readonly shared: LogChannelState;
private readonly name: string;
private readonly isRoot: boolean;
- private constructor(sock: Socket, name: string, isRoot: boolean) {
- this.sock = sock;
+ private constructor(shared: LogChannelState, name: string, isRoot: boolean) {
+ this.shared = shared;
this.name = name;
this.isRoot = isRoot;
- if (isRoot) {
- sock.on("error", (err) => {
- process.stderr.write(`[${this.name}] log socket error:
${err.message}\n`);
- });
- }
}
static async connect(addr: string, name: string = DEFAULT_LOGGER_NAME):
Promise<LogChannel> {
- return new LogChannel(await connectTcp(addr), name, true);
+ const shared: LogChannelState = {
+ sock: await connectTcp(addr),
+ connected: true,
+ closed: false,
+ };
+ shared.sock.on("error", (err) => {
+ process.stderr.write(`[${name}] log socket error: ${err.message}\n`);
+ });
Review Comment:
The `"error"` handler only writes to stderr; only the later `"close"`
handler flips `shared.connected = false`. Node fires `"close"` a tick or so
after `"error"` (empirically ~0.05ms later in a local repro), and in that gap a
`send()` call still takes the "connected" branch and writes to an
already-destroyed socket — the record is silently dropped instead of falling
back to stderr as intended. Setting the flag in the `error` handler too closes
the gap:
```suggestion
shared.sock.on("error", (err) => {
shared.connected = false;
process.stderr.write(`[${name}] log socket error: ${err.message}\n`);
});
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
