Is there a race condition bug?

[godhand4826]

I was wondering about how this project encapsulate the relational database transaction. The definition of good repository interface (/internal/app/service/barter/interface.go/GoodRepository) might can't get away from race condition. Before using UpdateGoods, there must be a GetGoodByID somewhere. Since the read and write are not in the same transaction, so the state is not protected by the relational database.

let's have a look at /internal/app/service/barter/exchange_service.go/(*BarterService).ExchangeGoods

• let's say the initial state is...
  • a good X belongs to trader A
  • a good Y belongs to trader B
  • a good Z belongs to trader C
• trader A launch two exchange request XY and XZ
• the final state is...
  • a good X belongs to trader C
  • a good Y belongs to trader A
  • a good Z belongs to trader A

I'm might be wrong because I'm not really execute it. I'm just simulate the execution in my head.

[JalexChang]

Hi, Indeed, the race condition may happen under the current version if multiple traders are concurrently using the app. We don't add validation/constraint things to the code because that may distract things we want to explain.

In the real world, I guess we will use a distributed locking mechanism (e.g. through redis, etcd, or firestore) to avoid the race condition.

func (s *BarterService) ExchangeGoods(ctx context.Context, param ExchangeGoodsParam) common.Error {
	// 1. Claim an event to exchange goods X and Y 
        s.lockServer.claim(X, Y, ttl)

        // 2. Check ownership of request good
	// ......

	// 3. Check the target good exist or not
	// ......

	// 4. Exchange ownership of two goods
        // .......

        // 5. Disclaim the event
        s.lockServer.disclaim(X, Y)

	return nil
}

Leveraging transactions also works but makes the implementation adapter function complex: application:

func (s *BarterService) ExchangeGoods(ctx context.Context, param ExchangeGoodsParam) common.Error {
        // 1. Check ownership of request good
	// ......

	// 2. Check the target good exist or not
	// ......

	// 3. Exchange ownership of two goods
        _, err = s.goodRepo.CheckOwnerIDsAndUpdateGoods(ctx, param )
        // ......
	return nil
}

adapter:

func (r *PostgresRepository) CheckOwnerIDsAndUpdateGoods(ctx context.Context,  param CheckOwnerIDsAndUpdateGoodsParam) (updatedGoods []barter.Good, err common.Error) {
	tx, err := r.beginTx()
	if err != nil {
		return nil, err
	}
	defer func() {
		err = r.finishTx(err, tx)
	}()
        // 1. Get goods again
       if  _, err  = r.getGoodByIDandOwnerID(ctx, tx, param.G1.ID, param.G1.OnwerID); err != nil { // ...}
       if  _, err  = r.getGoodByIDandOwnerID(ctx, tx, param.G2.ID, param.G2.OnwerID); err != nil { // ...}
       
        // 2. Update Goods
	for i := range goods {
		updatedGood, err := r.updateGood(ctx, tx, param.updateGoods[i])
		if err != nil {
			return nil, err
		}
		updatedGoods = append(updatedGoods, *updatedGood)
	}

	return updatedGoods, nil
}

[JalexChang]

@godhand4826 thanks for your comments! I guess the user story was truly not well written. We will try finding other samples to avoid misleading 💪

[godhand4826]

@JalexChang Thanks for your reply.😘 Distributed locking mechanism might be a little bit violate ddd, since lockServer is not business logic(concept). Also required to mock lockServer when perform unit testing. Leveraging transactions might be better. But the business logic will be implemented at repository layer which is ddd trying to avoid. Here is another way to do it (but makes the adapter interface more complex😭). Inject the use-case behavior into the repository method to keep business logic clean. Let the repository implementation decide which data synchronization mechanism(redlock, transaction...) should use. No matter an error occurred at repository layer or business layer. Since the common.Error is a flatten entity. It's still very simple to handle. 💪

application:

func (s *BarterService) ExchangeGoods(ctx context.Context, param ExchangeGoodsParam) common.Error {
    g1, g2, err := s.goodRepo.UpdateTwoGoods(ctx, param.G1, param.G2, func(g1, g2 barter.Good) common.Error {
        // 1. Check ownership of request good
        // ......

        // 2. Check the target good exist or not
        // ......

        // 3. Exchange ownership of two goods
        // ......
        return nil
    })

    return err
}

adapter:

func (r *PostgresRepository) UpdateTwoGoods(
    ctx context.Context,
    id, id2 string,
    updateFn func(*barter.Good, *barter.Good) (*barter.Good, *barter.Good, common.Error),
) (barter.Good, barter.Good, err common.Error) {
    tx, err := r.beginTx()
    if err != nil {
        return nil, err
    }
    defer func() {
        err = r.finishTx(err, tx)
    }()

    // 1. Fetch required data
    if g1, err  = r.getGoodByID(ctx, tx, id); err != nil { // ... }
    if g2, err  = r.getGoodByID(ctx, tx, id2); err != nil { // ... }

    // 2. Apply business logic (usecase)
    if g1, g2, err = updateFn(g1, g2); err != nil { // ... }

    return g1, g2, nil
}

[JalexChang]

@godhand4826 Thanks for your suggestion!

Your proposed solution is much more elegant, leveraging the benefits of transactions but not breaking the boundaries between application and adapter. That's awesome! The callback way is commonly introduced in some DDD projects, such as wild workouts. And we did discuss the callback way at the very beginning when figuring out how to introduce DDD to teams. However, the style was a little bit hard to understand for some members, struggling with differentiating its use cases. So, we are still working on adopting the style into our architecture 😭.

Do you mind sharing more experiences and perspectives with us 😃 ?

[david7482]

@godhand4826 I think it's an excellent approach. Would you mind opening a PR to address your idea?

[godhand4826]

I'm just read some pages of DDD books and blogs. They love DDD, and trying to share the good part of it. But just like talking about clean architecture, most of them didn't mentioned about language specific detail. It's reasonable that they aren't talking about the details because the topic was DDD. But talking about how to bring them into a specific programming language is more direct way to persuasive that DDD is practical and really helped. Unfortunately, there is still lack of resource. Instead of fundamental of clean architecture(Another hard part to introduce to teams😵), I was expected more language specific content (topic, experiences and solution) when apply DDD with golang. (It's seems like java is more capable with ddd🤔) Such like...

• Using Code-gen to handle tedious rounting, parsing json, http request code. and generalize the Repostory interface and implementation. (I'm still waiting golang 1.18 generic)
• How to encapsulate some technical detail with golang?(interface of cache, message queue, transaction, and where should they go)
• Project folder structure, and how to make sure developer didn't break the rules.
• Finite-state Machines and Enum alternative in golang.
• Mock-gen, Dependency injection (Things that JAVA guy familiar with).
• Performance sensitivity usecase optimization.
• Some open issues that gophers are still struggling.

[sj82516]

This awesome discussion! I encountered the same issue when I try to implement clean architecture.
My Scenario is quite similar

// check product stock
// check user account balance
// -> do some business check
// minus stock and minus balance

These four step should be wrapped in one transaction but third step is indeed business logic.

Instead of using callback, I am wondering could I directly call usecase in repo ?
Since repo is in third layer and usecase is in second layer. directly calling usecase in repo would not break dependency rule. Although the responsibility of repo would be messy because there are some usecase mixed in the repo.

[godhand4826]

@sj82516 Imagine you have multiple different use-cases and multiple repository implementations. If you want to directly call use-case in repo, which use-case should you invoke? Let controller parse the request and sending enum(command) as argument to repository to decide which use-case to use? That might be a long switch-case for each use-case inside of each repository implementations repeatedly. Adding new use-case is hard too.

In my opinion, the point of DDD is talking about modeling business logic and communicate with different kinds of expert. Defining repo interface for CURD operation and some essential transactional (NOT ONLY relational database transaction) operation is general enough. The abstraction(interface) is definitely not the perfect one, if considering...

• more complex query (e.g. GetByName,GetByIDSortByName, GetManyGtThan... ) (But for read only operation, considering break the abstraction and direct access after authenticated.)
• database dependent operations (require functionality or performance that can only be fulfilled by specific database) (e.g. nested relational database transaction, no-sql horizontal scaling...)
• It might never need(have to) to change database. (Still a good way to manage source code to keep the logic clean, easy to test and simple to read👍)

[sj82516]

Yes Make sense! Thanks @godhand4826