zellij-org / zellij

A terminal workspace with batteries included
https://zellij.dev
MIT License
20.61k stars 638 forks source link

Long session name hangs zellij #2591

Closed Lilja closed 1 year ago

Lilja commented 1 year ago

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

The latter one is 37 characters long

imsnif commented 1 year ago

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.

deepsghimire commented 1 year ago

I would like to contribute to this issue

imsnif commented 1 year ago

Go for it :)

deepsghimire commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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

ghost commented 1 year ago

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 commented 1 year ago

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

opax commented 6 months ago

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 commented 2 months ago

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