Critical bug in MCServer

[FakeTruth]

See this bug report: http://mc-server.org/support/index.php?d...ask_id=125

I don't know how to fix this. Please help

[mtilden]

Not on the up and up with threads and thread safe sorry to say.

[Lapayo]

Maybe we could do something like "GetSecureChunk" which returns the requested chunk and put it into a list of chunks which are not allowed to be destroyed.

When the chunk operation is done the client could call "UnsecureChunk" or something like this and the Chunk could probably be destroyed.

Just a thought. Have never tried this

[FakeTruth]

That's an idea.. but I think it would be faster to just store some kind of reference count in the chunk itself, otherwise chunks are added and removed from the list all the time, which is probably slow, and checking whether a chunk is in the list is also slow.

Both ways require the same amount of hassle though.. Always cleaning up after you're done with the chunk

[xoft]

We could always handle chunk pointers as std::tr1::shared_ptr<cChunk>.

Though a colleague of mine says "shared pointers and reference counting is the poor man's solution to poor man's design - you should instead design so that you always know who owns the object and who's responsible for destroying it." and over time I've come to agree with him. There are only a very few situations where reference-counting is really necessary, and by redesigning our code not to use it, we were able to eliminate 99 % of weird bugs reported with our app.

[FakeTruth]

I agree, but of course you need to come up with such a design first

I never intended MCServer to be multithreaded when I started working on it.

[xoft]

Well, I'm working on it Re-threading cClientHandles was one of the first steps needed.

That was probably the worst design decision, but hey, no-one can predict where a piece of software will go in a few years. I still work on a codebase founded 20 years ago and by then, multi-cure didn't exist so multithreading was considered useless. What a pity.

[xoft]

Okay, I dug really deep into this and I think it's working out nicely. I've decided to make a shared pointer to cChunk to be the entity that is passed to and from routines. I'm thread-locking everything up to protect against race conditions. This also requires that many of the interface functions be rewritten. The changes are really massive, I think I've re-written about 30 % of the entire codebase by now, and still not finished. I hope no-one commits anything big to the repository right now, or else I'll choke on the conflicts

The main idea that I'm trying to implement is that the code doesn't get a direct cChunk pointer anywhere. Anyone asking for a chunk will get a shared pointer, and a valid one. This way there's no problem with a thread accessing a chunk when another thread has deleted it. However, it requires that all code checks the returned pointer whether the chunk is valid (loaded / generated) and act accordingly if it is not.

I've run into a few cases that'll need rewriting for thread safety as well - clients in a server, entities in a world, block entities in a chunk - these all will need a similar set of changes in the near future. I'm leaving them as-is for now - it worked till now, it'll probably still work, though with the occasional danger of a crash.

[FakeTruth]

The main idea that I'm trying to implement is that the code doesn't get a direct cChunk pointer anywhere. Anyone asking for a chunk will get a shared pointer, and a valid one. This way there's no problem with a thread accessing a chunk when another thread has deleted it. However, it requires that all code checks the returned pointer whether the chunk is valid (loaded / generated) and act accordingly if it is not.

I thought I already did something like this, the cChunk_ptr class.

[xoft]

Yeah, but it was not used consistently across all code, so it came out pretty useless. Not to mention a home-cooked shared-ptr would probably have had some problems

Also, I need to close up many objects - instead of the sequence
CCC->LockAAA(),
CCC->GetAAAXList(),
for each A in AAA call A->DoBBB(),
CCC->UnlockAAA(),
the objects need to hide their internals and let others use
CCC->DoBBBOnAAAList()
instead.