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.

Sign up for GitHub

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

test(daemon): Improve cross-platform sockets using pid suffix #1620

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

test(daemon): Improve cross-platform sockets using pid suffix #1620

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 3 additions & 2 deletions packages/daemon/test/test-endo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import '@endo/lockdown/commit-debug.js';
import test from 'ava';
import url from 'url';
import path from 'path';
import os from 'os';
import { E } from '@endo/far';
import { makePromiseKit } from '@endo/promise-kit';
import {
Expand All @@ -29,8 +30,8 @@ const locator = {
cachePath: path.join(dirname, 'tmp/cache'),
sockPath:
process.platform === 'win32'
? raw`\\?\pipe\endo-test.sock`
: path.join(dirname, 'tmp/endo.sock'),
? raw`\\?\pipe\endo-test-${process.pid}.sock`
Copy link
Member

@kriskowal kriskowal Jun 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I’m curious whether the problem can be better addressed by ensuring that Endo stomps over the existing named pipe, since restarting without a graceful shutdown will affect end users as well.

Copy link
Member

Choose a reason for hiding this comment

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

Or, if handling the named pipe gracefully on Windows is not in the cards, we may need to adjust some other behavior around @endo/where for discovering the rendezvous location of the Endo named pipe by looking first at the PID file to infer the PID in the named pipe.

: path.join(os.tmpdir(), `endo-test-${process.pid}.sock`),
Copy link
Member

Choose a reason for hiding this comment

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

Using an OS tmpdir makes it harder to inspect the logs when the tests fail. If we go this route, there at least needs to be a t.log so the developer can rendezvous with the dump, but I’m inclined to keep it local. There’s a corresponding entry in .gitignore.

Copy link
Member

@michaelfig michaelfig Jun 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Isn't this just the socket path, and nothing else? Or is there some bizarre derivation of the log path from the socket path I'm missing?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see. You are correct. This only affects the location of the socket or named pipe, not the log dump.

};

test.serial('lifecycle', async t => {
Expand Down