8 ms·
The case of the critical section that let multiple threads enter a block of code
- davydm 2y agoOr rather, "the case of a buggy lazy-init function which reinitialized the critical section every time"
- akoboldfrying 2y agoLoose typing strikes again. I understand the temptation of "Let's just use an int return type, that way later on we can easily let them indicate different flavours of error if we want". But then you leave yourself open to this. The full-BDSM approach is for every component Foo that works with callbacks like this to define its own FooResult type with no automatic type conversions to it. In C, enum suffices, though a determined idiot can just cast to override; using FooResult as defined below makes it harder to accidentally do the wrong thing: enum FooResultImpl { FOO_SUCCESS_IMPL, FOO_FAILURE_IMPL }; struct FooResult { enum FooResultImpl USE_A_CONVERSION_FUNCTION_INSTEAD_OF_ACCESSING_THIS_DIRECTLY; } FOO_SUCCESS = { FOO_SUCCESS_IMPL }, FOO_FAILURE = { FOO_FAILURE_IMPL }; (Yes, it's still possible to get an erroneous value in there at initialisation time without having to type out "USE_A_CONV..." -- but this is C, you can only do so much. You can't even prevent a FooResult variable from containing uninitialised garbage...)
- emn13 2y agoIs the advantage over an enum not kind of small? We're seeing bugs here because people tried to do the right thing but the tooling has absolutely no way of helping anybody to do that. Simply preventing accidental mistakes would prevent these. Adding complexity to make it harder (though never impossible) for consumers to misuse the API in a complex way seems like it's potentially going to far. Then again, it's been years since I used this kind of C, so maybe my instincts are rusty here (no rust-pun intended!)
- akoboldfrying 2y agoYou're right, this may be overkill. OTOH, casting between integer types in C can (unfortunately) feel like clicking away confirmation dialog boxes -- something too readily done without full understanding ("Oh, it's always just 1 or 0, of course it will fit in the target type [so no need to think further]"). While it's annoying boilerplate for the Foo component to have to write, I don't think clients of Foo see much additional complexity. They can still write "return FOO_SUCCESS;", etc.
- magicalhippo 2y agoI kinda like the way Boost did error_code[1], which got incorporated into C++11[2]. Essentially you got a generic error_code struct which has two members, an int to hold a given error value or zero if there's no error, and a reference to an error category which helps interpret the error value. Effectively the error category is an interface, so in C terms it would be a reference to an error_category struct filled with function pointers. There's then some machinery which allows you to compare specific error codes to generic error conditions like "file not found", abstracting away the specifics of the error code. It's not problem free[3], but I've used this pattern in languages like Pascal and felt it worked well for me. [1]: https://www.boost.org/doc/libs/1_82_0/libs/system/doc/html/system.html https://www.boost.org/doc/libs/1_82_0/libs/system/doc/html/s... [2]: https://en.cppreference.com/w/cpp/header/system_error https://en.cppreference.com/w/cpp/header/system_error [3]: https://akrzemi1.wordpress.com/2017/10/14/error-codes-some-clarifications/ https://akrzemi1.wordpress.com/2017/10/14/error-codes-some-c...
- alfiedotwtf 2y agoYes, a “Sum” type in Type Theory
- magicalhippo 2y agoWell it's more like a dynamically defined sum type, no? There's also the scaffolding around it, like the way error codes compare for equivalence[1] against error conditions in a symmetric way[2]. The result is you can easily add new a domain-specific error category, and error codes using your new category can be fed to existing code and they'll do something sensible without further modification. Your code with the new category could even be loaded at runtime[3]. Not something you can do with plain sum types, ie tagged unions in C. At least as far as I know, though I'm no expert. [1]: https://www.boost.org/doc/libs/1_82_0/libs/system/doc/html/system.html#usage_testing_for_specific_error_conditions https://www.boost.org/doc/libs/1_82_0/libs/system/doc/html/s... [2]: https://www.boost.org/doc/libs/1_82_0/libs/system/doc/html/system.html#ref_comparisons_2 https://www.boost.org/doc/libs/1_82_0/libs/system/doc/html/s... [3]: though there might be dragons, see my previous reference
- tialaramex 2y agoI think I don't understand why they're making a critical section at all. The end goal is to initialize something no more than once, right? But the technology they're using (wrongly, but it did exist and they were clearly aware of it) to make a critical section does initialize a thing exactly once. I also don't understand the use of SRWLock here, or rather, I sort of do but it's a hole in Microsoft's technology stack. SRWLock is a complicated (and as it turns out, buggy, but that's not relevant here) tool, but all we want here is a mutex, so we don't actually need SRWLock except that Microsoft doesn't provide the simple mutex, so this really is what you'd have written at the time† - conjure into existence the over-complicated SRWLock and just don't use most of its functionality. † Today you should do the same trick as a Linux futex although you spell it differently in Windows. It's also optionally smaller, which is nice for this sort of job, a futex costs 4 bytes but in Windows we can spend just one byte.
- Wumpnot 2y agoSRWLock perf is slightly better than Window WaitOnAddress stuff, and works on older versions of windows.
- tialaramex 2y agoI find this unlikely. Do have some real world evidence for this? Microbenchmarks are not at all useful for this stuff in my experience. For speed: In the uncontended case, which is what we mostly care about because if you're contended it's game over for speed, they're both a single atomic CAS, so that's no difference. In terms of size, the pointer is bigger than you'd want - it's no pthread_mutex or the analogous Windows data structure where it's a multi cache line disaster of a data structure - but it's clearly worse than a futex or WaitOnAddress solution.
- Wumpnot 2y agoA few years ago I compared them, it was not a microbenchmark, but a real application. There were a few million(almost entirely uncontended) exclusive locks being taken on startup, SRWLock was consistently faster, though the difference was not large.
- hyperhello 2y agoIt looks Windows is lousy with callbacks and APIs that put the burden of understanding everything on the user, and some of Windows uses 0 to mean success, and some of Windows doesn't.
- colanderman 2y ago> some of Windows uses 0 to mean success, and some of Windows doesn't. This is unfortunately true of Unixes as well.
- jstimpfle 2y agoUnix APIs return -1 on error pretty consistently. The error code can then be read from the errno thread local variable. In the Linux kernel internal APIs (and probably others), -errno is returned directly (no errno mess), which is still negative. The one "Unix" API I know that returns > 0 on error is pthread, which returns +errno directly (but still 0 on success). Which APIs return 0 on error? I can't think of any.
- colanderman 2y ago`malloc(3)`. Many of the functions in string.h.
- jstimpfle 2y agoYeah there are a couple of (3) functions (i.e. not syscall interfaces) that return pointers. It's very common for those to return NULL on error, hardly a surprise. It will also blow up with a segfault should you forget to check success.
- School-Cotton 2y ago> It will also blow up with a segfault should you forget to check success. My guess is this is usually true in practice, but in C and C++ dereferencing a null pointer is UB so you really can't assume that.
- robmccoll 2y agoLooking at Microsoft's C code makes my eyes hurt. I don't know if it's the style (bracket placement, no new lines), naming conventions, typedeffing away pointers, or what, but it just doesn't read easily to me.
- pavlov 2y agoThe combination of all-caps type names and Hungarian notation for variables (“ppszOutStr”) makes it feel like you’re having a conversation with the vampire from the 2024 Nosferatu remake. He’s kind of yelling slowly and kind of talking in some East European language, and yet you can kind of understand what he’s saying. (And if I ever have to open an MFC codebase again, I’m going to be thinking of how Nosferatu springs up naked and rotting from his coffin)
- tialaramex 2y agoNotably it's Systems Hungarian which makes no sense whatsoever. This notation is a way to mention the kind of a variable in languages which don't directly express that. It starts in BCPL which doesn't have types, so if boop is a boolean and blip is a counter we need to annotate the name of the variable as the compiler doesn't see any reason you shouldn't use boop as a counter and blip as a boolean, so we maybe call them bBoop and cBlip or whatever. Now for the team writing say Excel, they have a typed language so they don't need to distinguish booleans from counters, but their language doesn't have or encourage distinct Row and Column types, those are both just integers, so "Apps Hungarian" uses the name to annotate variables with such information, clInsert is the column while rwInsert is the row, if I am reviewing code which is to inspect columns and it checks clFooC, clBar and rwBaz well why is it using a row number for a column? That warrants closer inspection. Unfortunately, this practice was divorced from its rationale and infected teams at Microsoft who had a typed language and didn't have kind information beyond that, but felt the need to use this notation anyway, producing "Systems Hungarian" where we mark out pFoo (it's a pointer named foo), and lBar (it's a long integer named bar). This is very silly, but at this point it has infested an entire division.
- 2y ago
- xyzzy9563 2y agoJust use strong typing and mutexes. This isn't rocket science.
- ddtaylor 2y agoMicrosoft take note that I read this article and everything Raymond Chen puts out under your company brand. I have zero interest in Windows as a platform and actively steer large customers away from it anytime it's discussed, since it has no value offering for most of us.
- psd1 2y agoYSK that someone has got into your account and posted dumb shit. Change your password.
- putzdown 2y agoI wake up every morning and thank God I am not working on or near Microsoft code. There is nothing about this code or anything about this story that is in any way sensible or pleasing. Take a simple, well-solved problem. Forgot all prior solutions. Solve it badly, with bad systems and bad ideas. Write the code in the ugliest, most opaque, most brittle and fragile manner imaginable. Now sit back and enjoy the satisfaction of getting to debug and resolve problems that never should have happened in the first place. The miracle is that Microsoft, built as it is to such a degree on this kind of trashy thinking and trashy source, still makes its annual billions. That right there is the power of incumbents.
- vijaybritto 2y agoI have a naive question here. Could this have been avoided if they had used Rust? Or is this a bug that can happen even in Rust code too?
- surajrmal 2y agoIt can be avoided in any compiled language that has a boolean type. That includes C these days. Unfortunately this functionality predates the existence of the boolean type.
- probably_wrong 2y agoThe original bug was returning STATUS_SUCCESS to indicate that a function had succeeded without noticing that STATUS_SUCCESS is defined as 0 in a function that's expected to return a non-zero value on success. This specific error could have happened on any language - defining two different return types and using the wrong one could happen in any language.
- andy12_ 2y ago> defining two different return types and using the wrong one could happen in any language This specifically is the kind of bug that is avoided with strong typing. The compiler screams at you when using the wrong return type. For example, if a callback expects a Result type, you must return a Result type, not some random int-like value whose definition of success and failure is arbitrary.
- commandlinefan 2y agoI didn't even have to click through the article to know that it would be Raymond Chen ; )