diff --git a/src/main/java/com/example/bankapp/controller/BankController.java b/src/main/java/com/example/bankapp/controller/BankController.java index 19fcded7..a72c92b5 100644 --- a/src/main/java/com/example/bankapp/controller/BankController.java +++ b/src/main/java/com/example/bankapp/controller/BankController.java @@ -64,7 +64,7 @@ public String withdraw(@RequestParam BigDecimal amount, Model model) { accountService.withdraw(account, amount); } catch (RuntimeException e) { model.addAttribute("error", e.getMessage()); - model.addAttribute("account", account); + model.addAttribute("account", accountService.findAccountByUsername(username)); return "dashboard"; } @@ -88,7 +88,7 @@ public String transferAmount(@RequestParam String toUsername, @RequestParam BigD accountService.transferAmount(fromAccount, toUsername, amount); } catch (RuntimeException e) { model.addAttribute("error", e.getMessage()); - model.addAttribute("account", fromAccount); + model.addAttribute("account", accountService.findAccountByUsername(username)); return "dashboard"; } diff --git a/src/main/java/com/example/bankapp/service/AccountService.java b/src/main/java/com/example/bankapp/service/AccountService.java index 5d7d90ec..e845997a 100644 --- a/src/main/java/com/example/bankapp/service/AccountService.java +++ b/src/main/java/com/example/bankapp/service/AccountService.java @@ -4,6 +4,9 @@ import com.example.bankapp.model.Transaction; import com.example.bankapp.repository.AccountRepository; import com.example.bankapp.repository.TransactionRepository; +import jakarta.persistence.EntityManager; +import jakarta.persistence.LockModeType; +import jakarta.persistence.PersistenceContext; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.security.core.GrantedAuthority; import org.springframework.security.core.authority.SimpleGrantedAuthority; @@ -12,6 +15,7 @@ import org.springframework.security.core.userdetails.UsernameNotFoundException; import org.springframework.security.crypto.password.PasswordEncoder; import org.springframework.stereotype.Service; +import org.springframework.transaction.annotation.Transactional; import java.math.BigDecimal; import java.time.LocalDateTime; @@ -31,6 +35,9 @@ public class AccountService implements UserDetailsService { @Autowired private TransactionRepository transactionRepository; + @PersistenceContext + private EntityManager entityManager; + public Account findAccountByUsername(String username) { return accountRepository.findByUsername(username).orElseThrow(() -> new RuntimeException("Account not found")); } @@ -48,33 +55,25 @@ public Account registerAccount(String username, String password) { } + @Transactional public void deposit(Account account, BigDecimal amount) { - account.setBalance(account.getBalance().add(amount)); - accountRepository.save(account); - - Transaction transaction = new Transaction( - amount, - "Deposit", - LocalDateTime.now(), - account - ); - transactionRepository.save(transaction); + Account locked = lockAccount(account.getId()); + locked.setBalance(locked.getBalance().add(amount)); + + transactionRepository.save(new Transaction(amount, "Deposit", LocalDateTime.now(), locked)); + account.setBalance(locked.getBalance()); } + @Transactional public void withdraw(Account account, BigDecimal amount) { - if (account.getBalance().compareTo(amount) < 0) { + Account locked = lockAccount(account.getId()); + if (locked.getBalance().compareTo(amount) < 0) { throw new RuntimeException("Insufficient funds"); } - account.setBalance(account.getBalance().subtract(amount)); - accountRepository.save(account); - - Transaction transaction = new Transaction( - amount, - "Withdrawal", - LocalDateTime.now(), - account - ); - transactionRepository.save(transaction); + locked.setBalance(locked.getBalance().subtract(amount)); + + transactionRepository.save(new Transaction(amount, "Withdrawal", LocalDateTime.now(), locked)); + account.setBalance(locked.getBalance()); } public List getTransactionHistory(Account account) { @@ -100,38 +99,42 @@ public Collection authorities() { return Arrays.asList(new SimpleGrantedAuthority("USER")); } + @Transactional public void transferAmount(Account fromAccount, String toUsername, BigDecimal amount) { - if (fromAccount.getBalance().compareTo(amount) < 0) { + Long toId = accountRepository.findByUsername(toUsername) + .orElseThrow(() -> new RuntimeException("Recipient account not found")) + .getId(); + + // Lock both rows in ascending id order so opposite-direction transfers cannot deadlock. + Long fromId = fromAccount.getId(); + Account first = lockAccount(Math.min(fromId, toId)); + Account second = lockAccount(Math.max(fromId, toId)); + Account from = fromId <= toId ? first : second; + Account to = fromId <= toId ? second : first; + + if (from.getBalance().compareTo(amount) < 0) { throw new RuntimeException("Insufficient funds"); } - Account toAccount = accountRepository.findByUsername(toUsername) - .orElseThrow(() -> new RuntimeException("Recipient account not found")); - - // Deduct from sender's account - fromAccount.setBalance(fromAccount.getBalance().subtract(amount)); - accountRepository.save(fromAccount); - - // Add to recipient's account - toAccount.setBalance(toAccount.getBalance().add(amount)); - accountRepository.save(toAccount); - - // Create transaction records for both accounts - Transaction debitTransaction = new Transaction( - amount, - "Transfer Out to " + toAccount.getUsername(), - LocalDateTime.now(), - fromAccount - ); - transactionRepository.save(debitTransaction); - - Transaction creditTransaction = new Transaction( - amount, - "Transfer In from " + fromAccount.getUsername(), - LocalDateTime.now(), - toAccount - ); - transactionRepository.save(creditTransaction); + from.setBalance(from.getBalance().subtract(amount)); + to.setBalance(to.getBalance().add(amount)); + + transactionRepository.save(new Transaction( + amount, "Transfer Out to " + to.getUsername(), LocalDateTime.now(), from)); + transactionRepository.save(new Transaction( + amount, "Transfer In from " + from.getUsername(), LocalDateTime.now(), to)); + fromAccount.setBalance(from.getBalance()); + } + + // refresh() with a lock issues SELECT ... FOR UPDATE and overwrites any stale copy already in the + // persistence context (open-in-view keeps the controller's Account managed for the whole request). + private Account lockAccount(Long id) { + Account account = entityManager.find(Account.class, id); + if (account == null) { + throw new RuntimeException("Account not found"); + } + entityManager.refresh(account, LockModeType.PESSIMISTIC_WRITE); + return account; } } diff --git a/src/test/java/com/example/bankapp/controller/BankControllerConcurrencyTest.java b/src/test/java/com/example/bankapp/controller/BankControllerConcurrencyTest.java new file mode 100644 index 00000000..7b916d1c --- /dev/null +++ b/src/test/java/com/example/bankapp/controller/BankControllerConcurrencyTest.java @@ -0,0 +1,151 @@ +package com.example.bankapp.controller; + +import com.example.bankapp.repository.AccountRepository; +import com.example.bankapp.repository.TransactionRepository; +import com.example.bankapp.service.AccountService; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.test.web.server.LocalServerPort; +import org.springframework.test.context.ActiveProfiles; + +import java.math.BigDecimal; +import java.net.URI; +import java.net.http.HttpClient; +import java.net.http.HttpRequest; +import java.net.http.HttpResponse; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.CompletableFuture; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Drives the real HTTP endpoints so the requests go through open-in-view, where the controller's + * Account stays managed for the whole request. + */ +@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT) +@ActiveProfiles("test") +class BankControllerConcurrencyTest { + + @LocalServerPort + private int port; + + @Autowired + private AccountService accountService; + + @Autowired + private AccountRepository accountRepository; + + @Autowired + private TransactionRepository transactionRepository; + + private final HttpClient http = HttpClient.newBuilder().followRedirects(HttpClient.Redirect.NEVER).build(); + + @BeforeEach + void cleanDatabase() { + transactionRepository.deleteAll(); + accountRepository.deleteAll(); + } + + @Test + void concurrentWithdrawRequestsCannotOverdraw() throws Exception { + accountService.registerAccount("mallory", "pw12345"); + String session = login("mallory", "pw12345"); + post(session, "/deposit", "amount=100"); + + fireConcurrently(20, i -> post(session, "/withdraw", "amount=100")); + + assertThat(balanceOf("mallory")).isEqualByComparingTo("0.00"); + assertThat(transactionRepository.findByAccountId(idOf("mallory"))) + .filteredOn(t -> t.getType().equals("Withdrawal")) + .hasSize(1); + } + + @Test + void concurrentTransferRequestsCannotSpendTheSameBalanceTwice() throws Exception { + accountService.registerAccount("mallory", "pw12345"); + for (int i = 0; i < 10; i++) { + accountService.registerAccount("accomplice" + i, "pw12345"); + } + String session = login("mallory", "pw12345"); + post(session, "/deposit", "amount=100"); + + fireConcurrently(10, i -> post(session, "/transfer", "toUsername=accomplice" + i + "&amount=100")); + + assertThat(balanceOf("mallory")).isEqualByComparingTo("0.00"); + BigDecimal received = BigDecimal.ZERO; + for (int i = 0; i < 10; i++) { + received = received.add(balanceOf("accomplice" + i)); + } + assertThat(received).isEqualByComparingTo("100.00"); + } + + @Test + void failedTransferRendersThePersistedBalance() throws Exception { + accountService.registerAccount("sender", "pw12345"); + String longName = "r".repeat(250); + accountService.registerAccount(longName, "pw12345"); + String session = login("sender", "pw12345"); + post(session, "/deposit", "amount=100"); + + String page = http.send(form(session, "/transfer", "toUsername=" + longName + "&amount=40"), + HttpResponse.BodyHandlers.ofString()).body(); + + assertThat(balanceOf("sender")).isEqualByComparingTo("100.00"); + assertThat(page).contains("Current Balance: $100.00").doesNotContain("Current Balance: $60.00"); + } + + private interface Request { + void send(int index) throws Exception; + } + + private void fireConcurrently(int count, Request request) { + List> futures = new ArrayList<>(); + for (int i = 0; i < count; i++) { + int index = i; + futures.add(CompletableFuture.runAsync(() -> { + try { + request.send(index); + } catch (Exception e) { + throw new IllegalStateException(e); + } + })); + } + CompletableFuture.allOf(futures.toArray(new CompletableFuture[0])).join(); + } + + private String login(String username, String password) throws Exception { + HttpResponse response = http.send(form(null, "/login", "username=" + username + "&password=" + password), + HttpResponse.BodyHandlers.discarding()); + assertThat(response.headers().firstValue("Location")).hasValueSatisfying(l -> assertThat(l).endsWith("/dashboard")); + return response.headers().allValues("Set-Cookie").stream() + .filter(c -> c.startsWith("JSESSIONID=")) + .map(c -> c.substring(0, c.indexOf(';'))) + .findFirst() + .orElseThrow(); + } + + private void post(String session, String path, String body) throws Exception { + http.send(form(session, path, body), HttpResponse.BodyHandlers.discarding()); + } + + private HttpRequest form(String session, String path, String body) { + HttpRequest.Builder builder = HttpRequest.newBuilder(URI.create("http://localhost:" + port + path)) + .header("Content-Type", "application/x-www-form-urlencoded") + .POST(HttpRequest.BodyPublishers.ofString(body)); + if (session != null) { + builder.header("Cookie", session); + } + return builder.build(); + } + + private Long idOf(String username) { + return accountRepository.findByUsername(username).orElseThrow().getId(); + } + + private BigDecimal balanceOf(String username) { + return accountRepository.findByUsername(username).orElseThrow().getBalance(); + } +} diff --git a/src/test/java/com/example/bankapp/service/AccountServiceConcurrencyTest.java b/src/test/java/com/example/bankapp/service/AccountServiceConcurrencyTest.java new file mode 100644 index 00000000..e88fef52 --- /dev/null +++ b/src/test/java/com/example/bankapp/service/AccountServiceConcurrencyTest.java @@ -0,0 +1,194 @@ +package com.example.bankapp.service; + +import com.example.bankapp.model.Account; +import com.example.bankapp.repository.AccountRepository; +import com.example.bankapp.repository.TransactionRepository; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.test.context.ActiveProfiles; + +import java.math.BigDecimal; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.Callable; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +@SpringBootTest +@ActiveProfiles("test") +class AccountServiceConcurrencyTest { + + @Autowired + private AccountService accountService; + + @Autowired + private AccountRepository accountRepository; + + @Autowired + private TransactionRepository transactionRepository; + + @BeforeEach + void cleanDatabase() { + transactionRepository.deleteAll(); + accountRepository.deleteAll(); + } + + @Test + void concurrentFullBalanceWithdrawalsLetExactlyOneSucceed() throws Exception { + Account mallory = createAccount("mallory", "100.00"); + + List results = runConcurrently(20, i -> { + Account copy = accountService.findAccountByUsername("mallory"); + accountService.withdraw(copy, new BigDecimal("100.00")); + }); + + assertThat(results.stream().filter(ok -> ok).count()).isEqualTo(1); + assertThat(balanceOf("mallory")).isEqualByComparingTo("0.00"); + assertThat(transactionRepository.findByAccountId(mallory.getId())).hasSize(1); + } + + @Test + void concurrentTransfersCannotSpendTheSameBalanceTwice() throws Exception { + createAccount("mallory", "100.00"); + for (int i = 0; i < 10; i++) { + createAccount("accomplice" + i, "0.00"); + } + + List results = runConcurrently(10, i -> { + Account copy = accountService.findAccountByUsername("mallory"); + accountService.transferAmount(copy, "accomplice" + i, new BigDecimal("100.00")); + }); + + assertThat(results.stream().filter(ok -> ok).count()).isEqualTo(1); + assertThat(balanceOf("mallory")).isEqualByComparingTo("0.00"); + BigDecimal received = BigDecimal.ZERO; + for (int i = 0; i < 10; i++) { + received = received.add(balanceOf("accomplice" + i)); + } + assertThat(received).isEqualByComparingTo("100.00"); + } + + @Test + void concurrentCreditsToOneRecipientAreNotLost() throws Exception { + createAccount("alice", "0.00"); + createAccount("bob", "1000.00"); + createAccount("mallory", "100.00"); + + List results = runConcurrently(21, i -> { + if (i == 0) { + Account bob = accountService.findAccountByUsername("bob"); + accountService.transferAmount(bob, "alice", new BigDecimal("1000.00")); + } else { + Account mallory = accountService.findAccountByUsername("mallory"); + accountService.transferAmount(mallory, "alice", new BigDecimal("0.01")); + } + }); + + assertThat(results).containsOnly(true); + assertThat(balanceOf("bob")).isEqualByComparingTo("0.00"); + assertThat(balanceOf("mallory")).isEqualByComparingTo("99.80"); + assertThat(balanceOf("alice")).isEqualByComparingTo("1000.20"); + } + + @Test + void concurrentDepositsAreAllApplied() throws Exception { + createAccount("alice", "0.00"); + + List results = runConcurrently(20, i -> { + Account copy = accountService.findAccountByUsername("alice"); + accountService.deposit(copy, new BigDecimal("5.00")); + }); + + assertThat(results).containsOnly(true); + assertThat(balanceOf("alice")).isEqualByComparingTo("100.00"); + } + + @Test + void oppositeDirectionTransfersDoNotDeadlockAndConserveMoney() throws Exception { + createAccount("alice", "500.00"); + createAccount("bob", "500.00"); + + List results = runConcurrently(20, i -> { + String from = i % 2 == 0 ? "alice" : "bob"; + String to = i % 2 == 0 ? "bob" : "alice"; + accountService.transferAmount(accountService.findAccountByUsername(from), to, new BigDecimal("10.00")); + }); + + assertThat(results).containsOnly(true); + assertThat(balanceOf("alice")).isEqualByComparingTo("500.00"); + assertThat(balanceOf("bob")).isEqualByComparingTo("500.00"); + } + + @Test + void failedTransferRollsBackBothBalancesAndRecords() { + Account sender = createAccount("sender", "100.00"); + // "Transfer Out to " + this name exceeds the 255-character transaction type column. + String longName = "r".repeat(250); + Account recipient = createAccount(longName, "0.00"); + + assertThatThrownBy(() -> accountService.transferAmount( + accountService.findAccountByUsername("sender"), longName, new BigDecimal("40.00"))) + .isInstanceOf(RuntimeException.class); + + assertThat(balanceOf("sender")).isEqualByComparingTo("100.00"); + assertThat(balanceOf(longName)).isEqualByComparingTo("0.00"); + assertThat(transactionRepository.findByAccountId(sender.getId())).isEmpty(); + assertThat(transactionRepository.findByAccountId(recipient.getId())).isEmpty(); + } + + private Account createAccount(String username, String balance) { + Account account = new Account(); + account.setUsername(username); + account.setPassword("x"); + account.setBalance(new BigDecimal(balance)); + return accountRepository.save(account); + } + + private BigDecimal balanceOf(String username) { + return accountRepository.findByUsername(username).orElseThrow().getBalance(); + } + + private interface Task { + void run(int index) throws Exception; + } + + private List runConcurrently(int threads, Task task) throws Exception { + ExecutorService pool = Executors.newFixedThreadPool(threads); + CountDownLatch ready = new CountDownLatch(threads); + CountDownLatch go = new CountDownLatch(1); + try { + List> futures = new ArrayList<>(); + for (int i = 0; i < threads; i++) { + int index = i; + Callable call = () -> { + ready.countDown(); + go.await(); + try { + task.run(index); + return true; + } catch (RuntimeException e) { + return false; + } + }; + futures.add(pool.submit(call)); + } + ready.await(10, TimeUnit.SECONDS); + go.countDown(); + List results = new ArrayList<>(); + for (Future f : futures) { + results.add(f.get(60, TimeUnit.SECONDS)); + } + return results; + } finally { + pool.shutdownNow(); + } + } +} diff --git a/src/test/resources/application-test.properties b/src/test/resources/application-test.properties index 4f249c2d..9c85ec48 100644 --- a/src/test/resources/application-test.properties +++ b/src/test/resources/application-test.properties @@ -1,4 +1,4 @@ -spring.datasource.url=jdbc:h2:mem:bankappdb;DB_CLOSE_DELAY=-1 +spring.datasource.url=jdbc:h2:mem:bankappdb;DB_CLOSE_DELAY=-1;LOCK_TIMEOUT=30000 spring.datasource.username=sa spring.datasource.password= spring.datasource.driver-class-name=org.h2.Driver