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

Dunst seems to be reusing IDs #1317

Open
Aster89 opened this issue Mar 17, 2024 · 14 comments
Open

Dunst seems to be reusing IDs #1317

Aster89 opened this issue Mar 17, 2024 · 14 comments

Comments

@Aster89
Copy link

Aster89 commented Mar 17, 2024

Issue description

It seems to me that Dunst violates the D-Bus protocol of the Desktop Notifications Specification, and specifically the part I've emphasised below:

If replaces_id is 0, the return value is a UINT32 that represent the notification. It is unique, and will not be reused unless a MAXINT number of notifications have been generated. An acceptable implementation may just use an incrementing counter for the ID. The returned ID is always greater than zero. Servers must make sure not to return zero as an ID.

If replaces_id is not 0, the returned value is the same value as replaces_id.

Indeed, Dunst seems to be using an incrementing counter as suggested, but the counter will not skip the values that have been already "consumed" because some non-0 replaces_id was passed.

Indeed, if I do

kill $(pidof dunst)

to ensure that Dunst it will be started anew on next notification, and then enter

notify-send 'hello' -p
notify-send 'hello' -p
notify-send 'hello' -p -r 5
notify-send 'hello' -p
notify-send 'hello' -p

I get these outputs

2
3
5
4
5

where the last 5 should have been 6, to avoid reusing the 5 that was explicitly provided by the 3rd notify-send call.

Installation info

  • Version: Dunst - A customizable and lightweight notification-daemon 1.10.0 (2024-02-19)
  • Install type: pacman -S dunst
  • Window manager / Desktop environment: XMonad
  • Distro: ArchLinux
Minimal dunstrc
# I don't think I have a dunstrc :/
# that means I'm using all defaults, right?
@bynect
Copy link
Member

bynect commented Mar 27, 2024

Would the least performance disrupting solution be store an array containing the used ids?

@Aster89
Copy link
Author

Aster89 commented Mar 27, 2024

Isn't simpler to store just the biggest used ID? Until the max is reached, at which point one would resent the max to 0. This would mean that some ids might be skipped, i.e. if right after starting the server you call notify-send 'hello' -p -r 500, 500 ids are gone. Would this be against the DNS?


As regards using an array, would a set be a better choice, considering tha the array might not be sorted? I'm assuming you mean an array where one pushes back the ids as they come.

@bynect
Copy link
Member

bynect commented Mar 28, 2024

Isn't simpler to store just the biggest used ID? Until the max is reached, at which point one would resent the max to 0. This would mean that some ids might be skipped, i.e. if right after starting the server you call notify-send 'hello' -p -r 500, 500 ids are gone. Would this be against the DNS?


As regards using an array, would a set be a better choice, considering tha the array might not be sorted? I'm assuming you mean an array where one pushes back the ids as they come.

I also thought about storing the max, but what if someone uses values close to the max uint32? We always skip many ids and reset which is kinda weird.

For the array I meant a simple int array. It should be faster than anything else (at least for normal sizes) because you don't have to hash anything (set and hashmaps are advantages for string).

@zappolowski what do you think about this one?

@zappolowski
Copy link
Member

I would consider using -r 5 without having a notification with this ID an error.

Citing from DNS again (Table 1. Notification Components):

Replaces ID An optional ID of an existing notification that this notification is intended to replace.

So, I'd say that a simple counter still should suffice, but we should check whether the requested replaces_id violates that stated requirement.

@Aster89
Copy link
Author

Aster89 commented Apr 3, 2024

Woooo, I missed that existing bit!

@bynect
Copy link
Member

bynect commented Apr 3, 2024

I would consider using -r 5 without having a notification with this ID an error.

Citing from DNS again (Table 1. Notification Components):

Replaces ID An optional ID of an existing notification that this notification is intended to replace.

So, I'd say that a simple counter still should suffice, but we should check whether the requested replaces_id violates that stated requirement.

So should we check if the given id is greater than the cureent counter and issue a warning if not?

@zappolowski
Copy link
Member

From my POV, there are two ways of dealing with it:

  1. just error out when no matching notification is found to be updated - the error reporting should be done via DBus in this case to make it obvious to the caller
  2. go on and just create a new notification, but don't use the supplied id for this, viz. create a new one

For the 2nd possibility one could also think about issuing a warning, but this might not be seen by the user depending on the configuration.

Maybe it's also worth looking into other notification daemons and how they deal with this case.

@Aster89
Copy link
Author

Aster89 commented Apr 6, 2024

So should we check if the given id is greater than the cureent counter and issue a warning if not?

What if all possible ids have been generated? You'd start all over with an id of 1, 2, ..., but that should not invalidate all the ids, no?

I mean

notify-send 'hello' -p # is given id 1
notify-send 'hello' -p # is given id 2
notify-send 'hello' -p # is given id 3
notify-send 'hello' -p -r 7 # should error/warning/whatever, because we're not there yet
notify-send 'hello' -p # is given id 4
...
notify-send 'hello' -p # is given id MAXINT-1
notify-send 'hello' -p # is given id MAXINT
notify-send 'hello' -p # is given id 1
notify-send 'hello' -p # is given id 2
notify-send 'hello' -p # is given id 3
notify-send 'hello' -p -r 7 # this is now valid... or not?

@bynect
Copy link
Member

bynect commented Apr 6, 2024

So should we check if the given id is greater than the cureent counter and issue a warning if not?

What if all possible ids have been generated? You'd start all over with an id of 1, 2, ..., but that should not invalidate all the ids, no?

I mean

notify-send 'hello' -p # is given id 1
notify-send 'hello' -p # is given id 2
notify-send 'hello' -p # is given id 3
notify-send 'hello' -p -r 7 # should error/warning/whatever, because we're not there yet
notify-send 'hello' -p # is given id 4
...
notify-send 'hello' -p # is given id MAXINT-1
notify-send 'hello' -p # is given id MAXINT
notify-send 'hello' -p # is given id 1
notify-send 'hello' -p # is given id 2
notify-send 'hello' -p # is given id 3
notify-send 'hello' -p -r 7 # this is now valid... or not?

The protocol says that if a maxint number of ids is reached we are allowed to restart from zero

@Aster89
Copy link
Author

Aster89 commented Apr 6, 2024

It reads

It is unique, and will not be reused unless a MAXINT number of notifications have been generated.

so it allows to reuse ids, but it does not mandate that all existing ids are invalidated at once, does it?

I mean, when you reach MAXINT, all ids are valid.

I expect id=1 to refer to some very old notification, so jumping back to 1 probably makes sense.

But invalidating all ids means that MAXINT is also invalidated, even if it you've maybe assigned it to a client a few seconds before, and the notification might be still visually visible, with the client trying to reuse it.

@bynect
Copy link
Member

bynect commented Apr 6, 2024

It reads

It is unique, and will not be reused unless a MAXINT number of notifications have been generated.

so it allows to reuse ids, but it does not mandate that all existing ids are invalidated at once, does it?

I mean, when you reach MAXINT, all ids are valid.

I expect id=1 to refer to some very old notification, so jumping back to 1 probably makes sense.

But invalidating all ids means that MAXINT is also invalidated, even if it you've maybe assigned it to a client a few seconds before, and the notification might be still visually visible, with the client trying to reuse it.

Mmh it doesn't seem too clear, however maybe we can keep a current id and a max id to keep track of it?

@Aster89
Copy link
Author

Aster89 commented Apr 6, 2024

Mmh it doesn't seem too clear

Let me clarify

notify-send 'hello' -p # is given id 1 (maybe several days ago?)
notify-send 'hello' -p # is given id 2
...
notify-send 'hello' -p # is given id MAXINT-1
notify-send 'hello' -p # is given id MAXINT (3 seconds ago)
notify-send 'hello' -p # is given id 1 (ok, it was old anyway)
notify-send 'hello' -p -r MAXINT # should we consider invalid? I think we shouldn't, as it's just 3 seconds old

however maybe we can keep a current id and a max id to keep track of it?

Yeah, I guess so.

@zappolowski
Copy link
Member

zappolowski commented Apr 7, 2024

What if all possible ids have been generated?

I consider reaching MAXINT practically impossible. If you manage to send 1k notifications per second you'd still need about 50 days to wrap around.

Another point is, that dunst just keeps a limited history of notifications (history_length with a default value of 20). Storing MAXINT notifications (to make replace_id work for an unbound history) would most probably trigger some memory issues (sizeof(struct notification) is 336 ignoring all pointees ... so, just assume something like 512 bytes (which probably doesn't suffice) and you'll end up with 2TiB of memory required just for dunst to store its history).

After giving some more thought, my suggestion is:

  • when replace_id is requested, we search for it in all queues (btw. it could reference a transient notification which doesn't end up in the history) in the list of currently shown notifications - changing history seems a bit weird in this case.
  • if it's found - no problem, update the notification and requeue it
  • if it's not found, just create a new notification with a new id (basically, act as replace_id = 0 was passed) and go on as normal.

@Aster89
Copy link
Author

Aster89 commented Apr 8, 2024

I consider reaching MAXINT practically impossible.

I hadn't thought about that either. I guess that might be a reason why the spec is not really clear about what to do?

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

No branches or pull requests

3 participants