hidapi/windows: thread local error messages

d3xMachina commented

2024-08-10 14:24:54 -05:00

(Migrated from github.com)

I've implemented thread local error messages on Windows to fix the double free crash when you have a Read and a Write happening in 2 different threads on the same device or when there is a thread for Read/Write for each device.

Need testing with multiple devices per thread since I only have 1 device per thread in my setup.

I've implemented thread local error messages on Windows to fix the double free crash when you have a Read and a Write happening in 2 different threads on the same device or when there is a thread for Read/Write for each device. Need testing with multiple devices per thread since I only have 1 device per thread in my setup.

Youw commented

2024-08-18 03:37:37 -05:00

(Migrated from github.com)

While this is an interesting idea to solve the current issue, there is unresolved issue with it:
If you're to use the device in different threads, you'll get several thread_local variables, but hid_close will free only one of them. There is a mem-leak by design here.

While this is an interesting idea to solve the current issue, there is unresolved issue with it: If you're to use the device in different threads, you'll get several thread_local variables, but hid_close will free only one of them. There is a mem-leak by design here.

d3xMachina commented

2024-08-18 07:39:28 -05:00

(Migrated from github.com)

Oh you're right, I'm working on a fix.

d3xMachina commented

2024-08-20 21:38:52 -05:00

(Migrated from github.com)

I've added a global list which contains the thread local allocations. When a thread wants to allocate memory for a thread local variable, it adds it to this list. The list is cleared as necessary. Accesses to the list are protected with a critical section for thread safety.

The only scenarios this global list is accessed are :

the first time an error message is created in a thread (to store the head of the list of errors)
when a thread is terminated (it removes the data corresponding to the thread)
when a device is closed (it removes the data corresponding to the device for all threads)
when hid_exit is called (it removes all the data)

One caveat :

hid_init must be called beforehand, otherwise the CriticalSection won't be initialized. Not sure how to get around that. Maybe I could simply initialize it in DllMain on a ProcessAttached event ?

I've added a global list which contains the thread local allocations. When a thread wants to allocate memory for a thread local variable, it adds it to this list. The list is cleared as necessary. Accesses to the list are protected with a critical section for thread safety. The only scenarios this global list is accessed are : - the first time an error message is created in a thread (to store the head of the list of errors) - when a thread is terminated (it removes the data corresponding to the thread) - when a device is closed (it removes the data corresponding to the device for all threads) - when hid_exit is called (it removes all the data) One caveat : - hid_init must be called beforehand, otherwise the CriticalSection won't be initialized. Not sure how to get around that. Maybe I could simply initialize it in DllMain on a ProcessAttached event ?

d3xMachina commented

2024-08-21 03:44:10 -05:00

(Migrated from github.com)

I've implemented my suggestion for the caveat I mentioned.

Youw commented

2024-08-22 12:28:09 -05:00

(Migrated from github.com)

Thanks for the implementation!

I totaly get your motivation for this change, but I have in mind a few doubts about this:

HIDAPI never been advertized as thread-safe;
The complexity of implementation is a bit higher than I'd be comfortable to accept and support;
Ultimately we have the same problem on other platforms and having it for Windows likely means porting (almost) the same imokementation for other backends.

I was even thinkg about an alternative implementation for this - instead of having a gloal list of thread-local vairables - have a list of per-device error_strings linked to a current thread-id. That should be a bit more simpler.

Another alternative is to introduce a hid_read_error API function, which would give only hid_read and hid_read_timeout errors - that implementation should be the most trivial on any platform, but breaks backward compatibility a bit...

Thanks for the implementation! I totaly get your motivation for this change, but I have in mind a few doubts about this: 1) HIDAPI never been advertized as thread-safe; 2) The complexity of implementation is a bit higher than I'd be comfortable to accept and support; 3) Ultimately we have the same problem on other platforms and having it for Windows likely means porting (almost) the same imokementation for other backends. I was even thinkg about an alternative implementation for this - instead of having a gloal list of thread-local vairables - have a list of per-device error_strings linked to a current thread-id. That should be a bit more simpler. Another alternative is to introduce a `hid_read_error` API function, which would give only hid_read and hid_read_timeout errors - that implementation should be the most trivial on any platform, but breaks backward compatibility a bit...

👍 1

d3xMachina commented

2024-08-22 15:13:15 -05:00

(Migrated from github.com)

HIDAPI never been advertized as thread-safe

While this is true, there is currently no way to have async read and write on a device without having to modify hidapi directly because of the current error message implementation. Right now you have to process them one by one or you risk a crash/undefined behavior. Having some thread safety guarantees is not a bad thing either. (the user still have to do their part, for example, the user must call hid_exit only when all threads are done using hidapi)

The complexity of implementation is a bit higher than I'd be comfortable to accept and support

Understandable!

Ultimately we have the same problem on other platforms and having it for Windows likely means porting (almost) the same imokementation for other backends.

Shouldn't be too hard as long as we find an equivalent to DllMain. I could put the tls code in its own files to avoid code duplication.

I was even thinkg about an alternative implementation for this - instead of having a gloal list of thread-local vairables - have a list of per-device error_strings linked to a current thread-id. That should be a bit more simpler.

I made an attempt here (not tested!) to see what it would look like. It's not much simplier imo (you have tls_get instead of tls_register mostly). And you have a potential lock contention each time error function are called (which is every times to set the error to null).

Another alternative is to introduce a hid_read_error API function, which would give only hid_read and hid_read_timeout errors - that implementation should be the most trivial on any platform, but breaks backward compatibility a bit...

Yeah, it's unfortunate.
Edit : maybe this could be implemented without breaking backward compatibility by having a function you call to enable separate error for read and write ? But that could be confusing for the user...

At the end of the day, you're the one who has the say. If you want to take a cautious approach, you could also make a separate branch (like for the connection callback) to check the stability over time and for those who need this.

> HIDAPI never been advertized as thread-safe While this is true, there is currently no way to have async read and write on a device without having to modify hidapi directly because of the current error message implementation. Right now you have to process them one by one or you risk a crash/undefined behavior. Having some thread safety guarantees is not a bad thing either. (the user still have to do their part, for example, the user must call hid_exit only when all threads are done using hidapi) > The complexity of implementation is a bit higher than I'd be comfortable to accept and support Understandable! > Ultimately we have the same problem on other platforms and having it for Windows likely means porting (almost) the same imokementation for other backends. Shouldn't be too hard as long as we find an equivalent to DllMain. I could put the tls code in its own files to avoid code duplication. > I was even thinkg about an alternative implementation for this - instead of having a gloal list of thread-local vairables - have a list of per-device error_strings linked to a current thread-id. That should be a bit more simpler. I made an attempt [here](https://github.com/d3xMachina/hidapi/tree/thread-local-error-messages-simplified) (not tested!) to see what it would look like. It's not much simplier imo (you have tls_get instead of tls_register mostly). And you have a potential lock contention each time error function are called (which is every times to set the error to null). > Another alternative is to introduce a hid_read_error API function, which would give only hid_read and hid_read_timeout errors - that implementation should be the most trivial on any platform, but breaks backward compatibility a bit... Yeah, it's unfortunate. Edit : maybe this could be implemented without breaking backward compatibility by having a function you call to enable separate error for read and write ? But that could be confusing for the user... At the end of the day, you're the one who has the say. If you want to take a cautious approach, you could also make a separate branch (like for the connection callback) to check the stability over time and for those who need this.

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Rows
Columns

hidapi/windows: thread local error messages #688

Pull request closed