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

Jump to bottom

test: Added relative path to accommodate limit #12601

Closed
wants to merge 3 commits into from
Closed

test: Added relative path to accommodate limit #12601

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f7a42ed
test: Added relative path to accommodate limit
Apr 22, 2017
b1f370b
test: improved call to path.relative()
Apr 27, 2017
d97a6d8
test: improved comments
Apr 27, 2017
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: 4 additions & 1 deletion test/parallel/test-net-connect-options-fd.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
const common = require('../common');
const assert = require('assert');
const net = require('net');
const path = require('path');
const Pipe = process.binding('pipe_wrap').Pipe;

if (common.isWindows) {
Expand Down Expand Up @@ -32,7 +33,9 @@ const forAllClients = (cb) => common.mustCall(cb, CLIENT_VARIANTS);

// Test Pipe fd is wrapped correctly
{
const prefix = `${common.PIPE}-net-connect-options-fd`;
// Using relative path on osx if the path is greaterthan 108 chars
// an AssertionError: -48 is thrown
Copy link
Member

Choose a reason for hiding this comment

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

nit: might be worthwhile adding a comment that explains what -48 means.

Copy link
Author

Choose a reason for hiding this comment

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

@jasnell That unfortunately I don't know. I believe @addaleax does?

Copy link
Member

@Trott Trott Apr 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I believe -48 is the libuv error for UV_EADDRINUSE. In any event, it's a libuv error.

I don't think this is OS X specific. It will happen on anything (or at least anything POSIX?) with a long enough path. I think you can just remove the comment (no one's going to wonder "why aren't they using an absolute path here??!!") or use a comment like this:

// Use relative path to avoid hitting 128-char length limit for socket paths in libuv.

/cc @addaleax to make sure I got that right (that it's 128 chars, that it's in libuv, etc.).

Copy link
Member

@addaleax addaleax Apr 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@Trott 108 should be the right number on Linux and almost certainly other systems too¹², quote unix(7):

struct sockaddr_un {
    sa_family_t sun_family;               /* AF_UNIX */
    char        sun_path[108];            /* pathname */
};

It’s this bit of code in libuv that truncates the path to that size. The actual value for EADDRINUSE is OS-dependent; for example, for me on Linux it’s 98 (which libuv forwards as -98).

See also #12708, I think that’s the same kind of problem.

¹ edit: yeah, your guess of “anything POSIX” sounds right, Windows doesn’t have UNIX sockets. ;)

² more edits: https://nodejs.org/api/net.html#net_server_listen_path_backlog_callback has numbers, it’s somewhere between 92 and 108.

const prefix = path.relative('.', `${common.PIPE}-net-connect-options-fd`);
const serverPath = `${prefix}-server`;
let counter = 0;
let socketCounter = 0;
Expand Down