SDL_GetStringRule for SDL_GetJoysticks(), etc.?

[slouken]

Should we return a const list of devices for functions like SDL_GetJoysticks()? The risk there is that people might be tempted to hold onto the list indefinitely.

@icculus, thoughts?

[icculus]

I mean, they'll figure out they can't when the list gets free'd on the next event pump. :)

I'm not against doing this.

[slouken]

Returning a list that must be freed makes it really clear that it's a temporary snapshot. On the other hand it makes the API easier to use if the caller doesn't have to free the memory. It also means we could return NULL if there are no devices, which is attractive from a user perspective.

I'm torn.

[icculus]

No devices should still return an empty, zero-terminated list, imho.

Even if we free the memory instead of the app, malloc can fail, and we need to return NULL for that.

If we own the memory, though, it means we can return the same pointer until the list changes instead of allocating and building it every time the app queries it, fwiw, making it possible for the call to be fast and lockless, if we so desired.

[slouken]

Hmmmmmmm

[slouken]

Okay, I'm sold. Let's do it!

[slouken]

Okay, I'm sold. Let's do it!

(I'm on it!) :)

[slouken]

Okay, so the full list of functions that will change is:
SDL_GetAudioPlaybackDevices()
SDL_GetAudioRecordingDevices()
SDL_GetCameraSupportedFormats()
SDL_GetCameras()
SDL_GetDisplays()
SDL_GetGamepads()
SDL_GetHaptics()
SDL_GetJoysticks()
SDL_GetKeyboards()
SDL_GetMice()
SDL_GetPreferredLocales()
SDL_GetSensors()
SDL_GetTouchDevices()
SDL_GetWindowICCProfile()

@icculus, I won't touch SDL_GetPens(), because I know that you have work in progress there.

[icculus]

Yeah, GetPens is going away. I'm working on that stuff now.

[slouken]

If we own the memory, though, it means we can return the same pointer until the list changes instead of allocating and building it every time the app queries it, fwiw, making it possible for the call to be fast and lockless, if we so desired.

I don't know if we can actually do that:

Thread A: main thread, pumps events
Thread B: calls SDL_GetDevices(), getting an internal pointer, gets preempted
Thread A: pumps events, changing the device list and setting it pending to be freed
Thread A: pumps events again, freeing the device list
Thread B: gets resumed, uses the internal pointer, crashes

This would result in very rare real-world crashes that will be hell to track down.

I think this potential race condition/crash pops up everywhere you've pointed at internal memory that is freed later in response to some other thread's action.

[slouken]

As far as I can see, the only way to prevent those kinds of crashes is for thread B to take a lock, copy the data, put it on the thread-local free queue, and unlock. I think we have to do that for every bit of state that we return from any API that can be called from multiple threads.

[slouken]

Either that or we have to reference count all that data, which we could certainly do...

[slouken]

I think the rule is "if it will be freed later, it needs to be copied when returned via API"

[icculus]

I don't know if we can actually do that:

Yeah, this occurred to me in the last hour or so.

I guess the policy is things that return data like this always makes a copy and immediately mark it with FreeLater, and the benefit is the app doesn't have to free it.

But I feel like we're one more unknown circumstance from this whole thing collapsing. Are we being too clever with all of this?

[slouken]

But I feel like we're one more unknown circumstance from this whole thing collapsing. Are we being too clever with all of this?

Very possibly. :)
Although not having to explicitly free data returned by the API is pretty nice, I'll go ahead and finish up this change and we can see what we think.

[andreasgrabher]

Stupid question:

I replaced this code in a project using SDL2:

	if (SDL_NumJoysticks() > 0)
		joy = SDL_JoystickOpen(0);

with this code for SDL3:

	int n;
	SDL_GetJoysticks(&n);
	if (n > 0)
		joy = SDL_OpenJoystick(0);

Does it mean I have to catch the returned pointer from SDL_GetJoysticks() just to free it?

[slouken]

Stupid question:

I replaced this code in a project using SDL2:

	if (SDL_NumJoysticks() > 0)
		joy = SDL_JoystickOpen(0);

with this code for SDL3:

	int n;
	SDL_GetJoysticks(&n);
	if (n > 0)
		joy = SDL_OpenJoystick(0);

Does it mean I have to catch the returned pointer from SDL_GetJoysticks() just to free it?

Not after #10311 is merged. :)

BTW, joy will always be NULL because 0 is an invalid ID. You do actually want to grab the joysticks so you know what the first joystick ID is:

 	int n;
 	SDL_Joysticks *joysticks = SDL_GetJoysticks(&n);
 	if (n > 0)
 		joy = SDL_OpenJoystick(joysticks[0]);

[slouken]

Okay, this is actually pretty awesome.

Each thread has a thread-local temporary memory pool. Events that are queued have their memory transferred from the local pool to be bound to the event while it's in the queue. Any event that is pulled from the queue has any associated memory transferred to that thread's local pool. This means that each thread can safely call SDL_FreeEventMemory() any time regardless of who's processing the event queue, and it means that threads can safely transfer temporary memory from one thread to another via the event queue.

Applications can also transfer ownership of temporary memory from SDL to the application, so they don't have to copy API results if they want to save them, and they can free temporary pointers individually if they want finer grained control over when temporary memory is freed.

Aaaaand, all of this is completely thread-safe. :)

Ready for review!

[slouken]

The only drawback I can think of here is that whether or not you can claim ownership of an API result becomes part of the ABI. I think that's fine, I'll just add it to the function documentation.

[slouken]

After discussion we'll just make all the GetStringRule APIs claimable and be done with it.