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

update the readme to indicate thread-safeness #11

Closed
wants to merge 1 commit into from

Conversation

tugberkugurlu
Copy link

I am essentially trying to asset my understanding with his change. When I looked at the doc, I saw this:

The generator-id-part of the Id is the part that you 'configure'; it could correspond to a host, thread...

Use of "thread" confused me here. Then I looked at the code, saw that an instance of an IdGenerator is thread-safe:

lock (_genlock)

@RobThree is my understanding correct here?

@RobThree
Copy link
Owner

RobThree commented Oct 9, 2018

It's currently thread safe because of the lock. However, I am planning to move some stuff out of the scope of the lock (because there's some work done in the lock that can be moved out) and maybe even get rid of the lock entirely.

What I meant to say / convey with the part you quoted from the readme is that, if you create a generator per thread (and configure the generators correctly), it is inherently thread safe (even without the lock) because every thread would have it's own generator-id.

The lock is currently in there to prevent people from accidentally generating duplicate ID's whenever an IdGenerator is shared between threads. In such scenario the lock works fine but when there are a few IdGenerators in a system on separate hosts for example the lock won't save you either from doing stupid unintentional things. And so I'm considering getting rid of the lock entirely to make it behave the same as if the generators were on different hosts / applications. The idea is, basically, that you, the user, ensure that there's only 1 generator per thread / host / application / whatever and then each generator is configured with a unique generator-id. Then no locks would be needed at all.

So while I do appreciate your PR, I don't think it'll get merged. If anything, I'm (considering to) going to get rid of the lock entirely and improve the documentation on how to use an IdGenerator correctly.

@RobThree RobThree self-assigned this Oct 9, 2018
@tugberkugurlu
Copy link
Author

tugberkugurlu commented Oct 9, 2018

And so I'm considering getting rid of the lock entirely to make it behave the same as if the generators were on different hosts / applications. The idea is, basically, that you, the user, ensure that there's only 1 generator per thread / host / application / whatever and then each generator is configured with a unique generator-id. Then no locks would be needed at all.

Hmm, I believe this would make the usage of this library hard and error-prone. This lib has a big used on a server application (e.g. ASP.NET Core hosted app) and within a single node, the expectation is to be able generate unique ids within that node with a singleton IdGenerator instance. This is not hard to get around, I can wrap this inside my own abstraction and lock the call there. However, you have to know that's the case. Alternative is to have a shared pool of IdGenerators per node which might be more performant but will be really ugly to maintain.

In such scenario the lock works fine but when there are a few IdGenerators in a system on separate hosts for example the lock won't save you either from doing stupid unintentional things.

I don't believe the thread scenario is the same as other cases. I would expect the library to handle this because it has all the power to be able to do this. However, it cannot do this across processes/nodes without requiring mode information (which is the generator-id).

What essentially drove you to make this decision? Was there feedback on this?

@RobThree
Copy link
Owner

RobThree commented Oct 9, 2018

What essentially drove you to make this decision?

Nothing; I hadn't (and haven't) made up my mind yet. I was recently thinking about it and considering it.

Was there feedback on this?

Nope; but I have some now 😉

It doesn't have to be black / white though, there are solutions where we can have best of both worlds where we simply implement IIdGenerator and have one implementation use the current, locking, method and another a lock-free method. We could keep the current behaviour and have a second implementation available. Or maybe even some factory method or builder pattern to create a generator with the desired properties (besides the MaskConfig).

But don't worry; I don't like breaking changes any more than anyone does and so I'm not planning on breaking / changing anything soon or without major notice. And I am open to all suggestions.

@tugberkugurlu
Copy link
Author

We could keep the current behaviour and have a second implementation available.

yep, I believe this makes a lot of sense. Not sure what the use-case would really be here though. I cannot think of a scenario where you would use an instance of an IdGenerator within a single-threaded environment in .NET context. However, if that scenario exists, having a type called LockFreeIdGenerator or something makes sense 👍

builder pattern to create a generator with the desired properties (besides the MaskConfig)

Like the idea of a builder pattern. It makes it easier to express the intent during the initialization.

Let me know if you need any help, I would be happy to :)

@tugberkugurlu tugberkugurlu deleted the patch-1 branch October 9, 2018 19:28
@RobThree
Copy link
Owner

RobThree commented Oct 10, 2018

I cannot think of a scenario where you would use an instance of an IdGenerator within a single-threaded environment in .NET context.

IdGen is used a lot as an alternative for GUID's/UUID's. The ID's are much friendlier for indices, half the (byte)size, incremental but "unpredictable" / "random". ID's tend to (unintentionally) give away some information at times. For example: let's say we have a forum with an /user/:id endpoint. In a classic scenario, with an incremental id, simply playing with the url makes it pretty simple to find the latest/last user-id. That (more-or-less) tells you the number of users in my system because "normal" id's tend to be incremental. In this scenario IdGen's id's aren't very much sequential and even a sort-of "random" (note: they are 100% predictable, deterministic) and, thus, doesn't tell you the number of users in my system.

However, if that scenario exists, having a type called LockFreeIdGenerator or something makes sense 👍

It also makes sense in a multi-threaded scenario. As long as you make sure each thread has it's own generator (with it's own generator-id (or "host id" if you will, even if it's on the same host) then there's no need for locking. Each thread can generate an id perfectly safe without the/a lock since each generator produces id's from it's own "range". In scenario's where you spin up threads yourself you could pass a unique idgenerator to each thread or, in scenario's where you don't control the threads, you could pull a generator from a pool of generators to avoid conflicts. Either way, they could do without the locks then.

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 this pull request may close these issues.

None yet

2 participants