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

Allow locks with arbitrary names #6

Open
ryanavella opened this issue Aug 28, 2022 · 4 comments
Open

Allow locks with arbitrary names #6

ryanavella opened this issue Aug 28, 2022 · 4 comments

Comments

@ryanavella
Copy link
Contributor

This would be a major breaking change and I'm not completely sure if I like it myself, but I would like to at least discuss it.

Imagine a hypothetical downstream project that creates locks using names that are only known at runtime. Let's say these names are strings with arbitrary characters. Then some calls to NamedLock::create will inevitably return Err(Error::InvalidCharacter).

This could be avoided if the strings were first encoded so that reserved characters ('\0', '/', and '\\') were transformed to non-reserved characters. The exact encoding scheme is a bikeshed I'm not ready to dive into at the moment, but something like percent-encoding or Base32 would fit the bill.

Pros:

  • We could possibly eliminate an Error variant from the public API
  • We can guarantee that lock creation can not fail due to invalid bytes
  • This can be done without additional allocations (depending on the choice of encoding scheme)

Cons:

  • This is a significant breaking change
  • The choice of encoding scheme is arbitrary, and every downstream user pays the penalty of that choice

Questions:

  • Would it make sense to "fix" NamedLock::create, or to make a new constructor method entirely?
  • Would we add an "escape hatch" for the current behavior?
    • If so, should it panic or return an error variant indicating invalid bytes?
@oblique
Copy link
Owner

oblique commented Sep 1, 2022

I understand the need but I don't like replacing characters or use special encoding. The main reason is because I want it to be very close to the low level representation of the locks, for example a C program can still use the flock system call to synchronize with this crate.

@ryanavella
Copy link
Contributor Author

What if, instead of a breaking change to the existing API, it were introduced as a new API entirely?

I'm imagining something like this:

/// Create/open a named lock by escaping the provided bytes.
///
/// Unlike [`create`], this will succeed even if the name
/// is empty, or if it contains `\0`, `/`, or `\`.
///
/// The escape sequence used is an implementation detail,
/// though it will produce a valid lock name on each platform,
/// and it is guaranteed that distinct names will not collide.
///
/// To avoid unintended collisions with locks
/// created through the [`create`] constructor,
/// the version 4 UUID `34e5115e-d2a8-4d57-b787-ffd5b3f0ca02`
/// is prepended to the escaped name, followed by a hyphen-minus (`-`). 
///
/// See [`create`] for more information on
/// platform-specific implementation details.
///
/// [`create`]: NamedLock::create
fn create_escaped(name: impl AsRef<[u8]>) -> NamedLock {
    todo!()
}

@oblique
Copy link
Owner

oblique commented Feb 28, 2024

Escaping is not supported, so in this case do you mean by replacing invalid characters with something else? For example with _?

@ryanavella
Copy link
Contributor Author

I just had a realization, it should be possible to escape arbitrary strings while addressing your earlier concern about interfacing with C programs.

This should work on both Windows and Unix, and allows empty strings, as well as '\0', '/', and '\\'. Any names that are currently allowed by NamedLock::create would remain unchanged.

fn escape(s: &str) -> String {
    #[cfg(unix)]
    const DELIM: char = '\\';
    
    #[cfg(windows)]
    const DELIM: char = '/';
    
    if s.is_empty() {
        return String::from(DELIM);
    }
    
    let mut escaped = String::with_capacity(s.len());
    for c in s.chars() {
        if matches!(c, '\0' | '\\' | '/') {
            escaped.push(DELIM);
            for b in c.escape_unicode().skip(1) {
                escaped.push(b);
            }
        } else {
            escaped.push(c);
        }
    }
    escaped
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants