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

try_lock() always reports "WouldBlock" #21

Closed
lbfs opened this issue Mar 31, 2024 · 6 comments
Closed

try_lock() always reports "WouldBlock" #21

lbfs opened this issue Mar 31, 2024 · 6 comments
Assignees
Labels
bug Something isn't working

Comments

@lbfs
Copy link

lbfs commented Mar 31, 2024

Hi!

I was playing around with this project and seeing if any of my code would work on a platform this old.

I tried this following snippet of code, but it always returns a "WouldBlock" failure. I see that on this version of the compiler, there are a few different Mutex implementations based on what support is available. Is the Mutex implementation that is used on Windows 98 incompatible with this try_lock() method?

    let mutex = Mutex::new(1);
    let guard = mutex.try_lock();

    match guard {
        Ok(_) => {
            println!("Got lock!");
        },
        Err(e) => {
            println!("{:#?}", e);
        }
    }
@RandomInsano
Copy link

RandomInsano commented Mar 31, 2024 via email

@lbfs
Copy link
Author

lbfs commented Apr 1, 2024

I'm using the standard library, I am not using a custom mutex implementation.

I tried forcing the different compatibility options for mutex implementations on Windows 98. What I discovered is that the legacy implementation allows my code to work as intended, but when I go into the second implementation ``CriticalSection``` which there is a note about not working on Windows 98 and other early Windows platforms. My application exhibits the same strange behavior it did previously with being unable to obtain a lock.

I think something is possibly wrong with the detection for this method?

https://github.com/rust9x/rust/blob/rust9x/library/std/src/sys/windows/locks/mutex/compat.rs#L21

@seritools seritools self-assigned this Apr 1, 2024
@seritools seritools added the bug Something isn't working label Apr 1, 2024
@seritools
Copy link
Member

seritools commented Apr 1, 2024

Awesome, thanks for the analysis.

Win98's kernel32.dll likely has an export for TryEnterCriticalSection that just fails (probably with GetLastError = unsupported), instead of not exporting it at all (just like with all the unicode W versions of APIs that take strings).

So you're right, the check is incorrect. Instead, the check for using CriticalSection for the mutex impl should likely hardcode a check based on https://github.com/rust9x/rust/blob/rust9x/library/std/src/sys/windows/compat/version.rs (= check for NT 4.0+).

For reference, from the old Platform SDK:

TryEnterCriticalSection
  Windows NT: Requires version 4.0 or later.
  Windows: Unsupported.
  Windows CE: Unsupported.
  Header: Declared in winbase.h.
  Import Library: Use kernel32.lib.

@seritools
Copy link
Member

@lbfs if you've got time, could you test the fix in https://github.com/rust9x/rust/tree/rust9x-1.76-beta (latest commit)?

Also tracking integrating that fix into the initial impl once rebasing rust9x to a new Rust version in the future #19

@lbfs
Copy link
Author

lbfs commented Apr 2, 2024

Previous behavior in my application
image

With your commit
image

So I think the detection is fixed!

Also, in my spare time I found this interesting article seeming to confirm the findings here, also talking about how this could be implemented on previous versions of Windows.
https://fanael.github.io/stockfish-on-windows-98.html#implementing-try-enter-critical-section

@seritools
Copy link
Member

seritools commented Apr 2, 2024

Haha, I remember that post/that workaround, it certainly should be possible, yeah. Another implementation for this can be found in the KernelEx compatibility layer: https://github.com/metaxor/KernelEx/blob/master/apilibs/kexbases/Kernel32/critsect.c#L161

For rust9x I've been trying to minimize additional, original code where possible, so adding code that relies on such internals is not something I'd want to do ^^

I'll release the new version shortly, likely today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants