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

pool.id uses ObjectIdentifier(connection) as the ID but that's not unique. #1849

Closed
weissi opened this issue Apr 3, 2024 · 4 comments · Fixed by #1852
Closed

pool.id uses ObjectIdentifier(connection) as the ID but that's not unique. #1849

weissi opened this issue Apr 3, 2024 · 4 comments · Fixed by #1852
Labels

Comments

@weissi
Copy link
Contributor

weissi commented Apr 3, 2024

ObjectIdentifier(something) is not unique. It's only unique at any given point in time. But it is not unique across the lifetime of a program. It prints the allocation address and after freeing one object, it will be available again to the next.

For example see how each of the 100 Class() instances has the same address/ObjectIdentifier

$ echo 'class Clazz {}; for _ in 0..<100 { print(ObjectIdentifier(Clazz())) }' | swift - | sort | uniq -c
 100 ObjectIdentifier(0x0000600000860bd0)
@weissi weissi added the bug label Apr 3, 2024
@glbrntt
Copy link
Collaborator

glbrntt commented Apr 4, 2024

Point taken but for the connection pool I think it's okay: each sub-pool (i.e. the thing we're identifying) is guaranteed lives for as long as the pool so they will be unique.

@weissi
Copy link
Contributor Author

weissi commented Apr 4, 2024

Point taken but for the connection pool I think it's okay: each sub-pool (i.e. the thing we're identifying) is guaranteed lives for as long as the pool so they will be unique.

What if the pool shuts down?

Also, the current ID wastes a lot of characters for nothing. I think a simple atomic counter like NIO/AHC do is better. Or UUID if you want globally unique.

@glbrntt
Copy link
Collaborator

glbrntt commented Apr 4, 2024

I'm not disagreeing with the premise of the issue. It's just low priority as pools are typically long-lived so IDs should be unique enough.

@glbrntt
Copy link
Collaborator

glbrntt commented Apr 9, 2024

Ended up doing this as part of #1852

@glbrntt glbrntt linked a pull request Apr 9, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants