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

Long session name hangs zellij #2591

Closed
Lilja opened this issue Jun 30, 2023 · 10 comments · Fixed by #2607
Closed

Long session name hangs zellij #2591

Lilja opened this issue Jun 30, 2023 · 10 comments · Fixed by #2607
Labels
good first issue Good for newcomers help wanted Extra attention is needed suspected bug

Comments

@Lilja
Copy link

Lilja commented Jun 30, 2023

zellij --session reallyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy works.
zellij --session reallyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy is hanging the program.

The latter one is 37 characters long

@imsnif
Copy link
Member

imsnif commented Jul 4, 2023

Nice find!

This is the error in the logs:

ERROR  |zellij_utils::errors::not| 2023-07-04 16:03:50.078 [server_listener] [zellij-utils/src/errors.rs:601]: Panic occured:
             thread: server_listener
             location: At zellij-server/src/lib.rs:281:25
             message: socket path should not be longer than 108 bytes

The fix for this should be validating session on the clap level in zellij_utils/src/cli.rs.

@imsnif imsnif added help wanted Extra attention is needed good first issue Good for newcomers labels Jul 4, 2023
@deepsghimire
Copy link
Contributor

I would like to contribute to this issue

@imsnif
Copy link
Member

imsnif commented Jul 4, 2023

Go for it :)

@deepsghimire
Copy link
Contributor

Hi! I was trying to reproduce the issue and the maximum length of session name that worked for me (no hang) was 78 characters. (need help here)
And the way I intended to validate the session is by defining the function:

fn  validate_session_length(name: &str) -> Result<String,String> {
    if name.len() > 78 {  // or whatever length that works
        Err(format!("Session name must be less than 78 bytes"))
    } else {
        return Ok(name.to_owned())
    }
}

at zellij_utils/src/cli.rs and using this function as value_parser for session of the CliArgs struct.

Do you know if this is the correct way to do this? Did I miss anything? Sorry for asking too many questions.

@imsnif
Copy link
Member

imsnif commented Jul 5, 2023

@deepsghimire - what I can say is that we want the user to get a proper error in the CLI, with coloring, the correct exit status and everything. The rest is up to you as the implementer.

I think the session name limit is platform specific (for me the number was 108) - so do look into that, we might need to be a little smarter about this.

Thanks for looking into this!

@ghost
Copy link

ghost commented Jul 5, 2023

In my case, the issue presents itself when the session length is at least 38 characters

@ghost
Copy link

ghost commented Jul 5, 2023

Nice find!

This is the error in the logs:

ERROR  |zellij_utils::errors::not| 2023-07-04 16:03:50.078 [server_listener] [zellij-utils/src/errors.rs:601]: Panic occured:
             thread: server_listener
             location: At zellij-server/src/lib.rs:281:25
             message: socket path should not be longer than 108 bytes

The fix for this should be validating session on the clap level in zellij_utils/src/cli.rs.

It looks like this error is outdated as master has a comment on the line the error quotes.

How can I get zellij to produce similar logs for me? I do not get this even when running zellij with the --debug flag.

@deepsghimire
Copy link
Contributor

@super-secret-github-user I used cargo xtask run

@opax
Copy link

opax commented Mar 4, 2024

It seems this is not really fixed; on my ARM mac with zellij 0.39.2, running

zellij -s 1234567890123456789012345678901234567

yields

ERROR  |zellij_utils::errors::not| 2024-03-04 19:03:25.984 [server_listener] [zellij-utils/src/errors.rs:632]: Panic occured:
             thread: server_listener
             location: At zellij-server/src/lib.rs:286:73
             message: called `Result::unwrap()` on an `Err` value: Custom { kind: InvalidInput, error: "socket path should not be longer than 104 bytes" }

The problem seems to be that the length of directory path of the socket is not properly taken into account. Since this path is based on the default temp directory, it tends to be much longer on macos machines than on linux machines.

In my case:

> echo -n $TMPDIR/zellij-501/0.39.2/ | wc -c
 68

Which leaves 104 - 68 = 36 characters as the maximal length for the session name. This nicely fits with my minimal error case, where the session name is 37 characters long.

By the way - the error triggered when I use a session name with 50 characters also does not seem to be quite right:

> zellij -s 12345678901234567890123456789012345678901234567890
error: Invalid value "12345678901234567890123456789012345678901234567890" for '--session <SESSION>': session name must be less than 0 characters

"session name must be less than 0 characters" is somewhat hard to comply to...

@pivoshenko
Copy link

Same here, on Mac with ARM the issue is still present and is very annoying 😬

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed suspected bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants