Extra Cookies

Yet Another Programmer's Blog

A Bug in a Java Servlet

We have a legacy system, which is a web service, receives HTTP POST from clients, parses the data, then stores them in a file.

The function of the system is simple, and people already done functional and performance test, it’s stable. As time drifted away, the system was copy and paste to some projects by only changing the data parsing logic.

I had a similar requirement recently, then I delved into the legacy code to check if it works in order to not reinventing the wheel.

WTF

At first, I noticed below code in a HttpServlet class, it allocates more than 1M memory for each HTTP POST request.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
private static final long MAX_CONTENT_LENGTH = 1024 * 1024;
private static final int BUFFER_SIZE = 4096;

...

public void doPost(HttpServletRequest request, HttpServletResponse response)
		throws ServletException, IOException {

    ...

    int requestContentBufSize = request.getContentLength() + MAX_CONTENT_LENGTH;
    ByteBuffer requestContentBuf = ByteBuffer.allocate(requestContentBufSize);
    byte[] buffer = new byte[BUFFER_SIZE];
    requestInputStream = new DataInputStream(request.getInputStream());
    int readBytes = 0;
    int totalReadBytes = 0;
    while ((readBytes = requestInputStream.read(buffer)) > 0) {
        requestContentBuf.put(buffer);
    	totalReadBytes = totalReadBytes + readBytes;
    }
    byte[] requestContent = Arrays.copyOf(requestContentBuf.array(), totalReadBytes);

    ...
}

It’s insane, I believe the memory should be the same as each HTTP POST body size. Then I changed the code.

1
int requestContentBufSize = request.getContentLength();

Deployed the service and sent one HTTP POST request to it.

curl -d 'Hello, World' http://my.server.com:9000/log

An Exception occurred.

The BufferOverflowException

After reducing the memory allocated for ByteBuffer, it overflows.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
java.nio.BufferOverflowException
	at java.nio.HeapByteBuffer.put(HeapByteBuffer.java:183)
	at java.nio.ByteBuffer.put(ByteBuffer.java:830)
	at com.myproject.servlet.LogServer.doPost(LogServer.java:99)
	at javax.servlet.http.HttpServlet.service(HttpServlet.java:643)
	at javax.servlet.http.HttpServlet.service(HttpServlet.java:723)
	at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:290)
	at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:206)
	at org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:233)
	at org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:191)
	at org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:127)
	at org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:103)
	at org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:109)
	at org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:293)
	at org.apache.coyote.http11.Http11Processor.process(Http11Processor.java:861)
	at org.apache.coyote.http11.Http11Protocol$Http11ConnectionHandler.process(Http11Protocol.java:606)
	at org.apache.tomcat.util.net.JIoEndpoint$Worker.run(JIoEndpoint.java:489)
	at java.lang.Thread.run(Thread.java:701)

I thought I’d better dig into how does the servlet do to make ByteBuffer get its data?

  1. It creates a small buffer occupied BUFFER_SIZE (4096) bytes.
  2. It iterates the HTTP request input stream, to put the data into the small buffer.
  3. It puts the small buffer to ByteBuffer and loop back to 1.

Well, in the last loop, the data read from the HTTP request input stream might smaller than the BUFFER_SIZE, but the servlet still puts BUFFER_SIZE bytes to ByteBuffer.

Then, to fix the ExceptionBufferOverflowException, I increased the capacity of previous ByteBuffer by BUFFER_SIZE.

1
int requestContentBufSize = request.getContentLength() + BUFFER_SIZE;

Deployed again, and

curl -d 'Hello, World' http://my.server.com:9000/log

The bug was fixed.

Did I?

The ServletInputStream

When client posts huge data, what could happen?

I created a String which is 7516 bytes, and sent to server.

curl -d 'very very long string' http://my.server.com:9000/log

Sometimes, the java.nio.BufferOverflowException occurred, and sometimes it didn’t.

What went wrong?

To find the root cause, I added some logs to trace the ByteBuffer.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
int requestContentBufSize = request.getContentLength() + BUFFER_SIZE;
ByteBuffer requestContentBuf = ByteBuffer.allocate(requestContentBufSize);
byte[] buffer = new byte[BUFFER_SIZE];
requestInputStream = new DataInputStream(request.getInputStream());
int readBytes = 0;
int totalReadBytes = 0;
log.debug("1: ByteBuffer position: " + requestContentBuf.position() +
        ", buffer capacity: " + requestContentBuf.capacity() +
        ", buffer remaining: " + requestContentBuf.remaining());
while ((readBytes = requestInputStream.read(buffer)) > 0) {
	requestContentBuf.put(buffer);
	totalReadBytes = totalReadBytes + readBytes;
    log.debug("2. Bytes read: " + readBytes);
    log.debug("1: ByteBuffer position: " + requestContentBuf.position() +
            ", buffer capacity: " + requestContentBuf.capacity() +
            ", buffer remaining: " + requestContentBuf.remaining());
}

The log printed when no exception,

1
2
3
4
5
- 1: ByteBuffer position: 0, buffer capacity: 11612, buffer remaining: 11612
- 2. Bytes read: 4096
- 1: ByteBuffer position: 4096, buffer capacity: 11612, buffer remaining: 7516
- 2. Bytes read: 3420
- 1: ByteBuffer position: 8192, buffer capacity: 11612, buffer remaining: 3420

The log printed when exception occurred,

1
2
3
4
5
- 1: ByteBuffer position: 0, buffer capacity: 11612, buffer remaining: 11612
- 2. Bytes read: 1356
- 1: ByteBuffer position: 4096, buffer capacity: 11612, buffer remaining: 7516
- 2. Bytes read: 1356
- 1: ByteBuffer position: 8192, buffer capacity: 11612, buffer remaining: 3420

Now, it is easy to find out the root cause is in these lines of code.

1
2
while ((readBytes = requestInputStream.read(buffer)) > 0) {
    requestContentBuf.put(buffer);

The read method call won’t put data to the buffer fully which was specified as 4096 bytes even when the input stream still has data.

And to fix it, just specify the offset and length of the small buffer.

1
2
while ((readBytes = requestInputStream.read(buffer)) > 0) {
    requestContentBuf.put(buffer, 0, readBytes);

I had increased the capacity of the ByteBuffer by BUFFER_SIZE, this change should also be reverted.

Now, the bug is fixed, and this is network programming.

Questions

“The system works a long time, and it shouldn’t have this problem or we knew it long ago”

This is because the client seldom posts data more than 4096 bytes to server.

“I have read the Javadoc of DataInputStream, the read method will put data fully to the specified buffer”

It didn’t, please read it again.

“I have tested the read method of DataInputStream on a file, it reads fully 4096 bytes in every iteration”

This is a web service, deploy it to a server and test.

“I have tested it on my local machine as a web service, and it reads fully 4096 bytes in every iteration”

This is a web service, it should be in a network.

At Last

When a potential bug was reported, we do tests to make it happen again and find the root cause.

We do not stop listening and just look for reasons to reject it.

When we find a bug, we do help others to make it reappear to collect information.

We do not sit there and just blame on others for their mistakes.

TDD on Swift

Long long ago, I wrote a post about how to do TDD using Objective-C, since Apple WWDC 2014, Swift is really eye-catching, I think I should write a new one to follow the trend.

XCTest is used as the unit test framework, and Xcode 6 is needed.

TDD Work-flow

  1. Add a test for a user case or a user story
  2. Run all tests and see if the new one fails
  3. Write some code that causes the test to pass
  4. Run tests, change production code until all test cases pass
  5. Refactor the production code
  6. Refactor the test code
  7. Return to 1, and repeat

The 5 and 6 are optional, do them only if needed, but be sure that DO NOT do them at the same time. That is, when you refactor production code, you can’t change the test code, until all the test cases are passed, then you are confident that your production code refactoring is perfect, then, you can refactor the test code, and this time, you can’t change the production code.

A Simple Example

We are about to implement a super simple bank account management tool.

Create a Project

Use Xcode to create a project BankAccount (iOS Single View Application)

Add a Test Case

Create a Swift file named SavingAccountTest, and choose BankAccountTests as target.

“People can deposit money to a saving account”, it’s our first user story.

1
2
3
4
5
6
7
8
9
10
11
import Foundation
import XCTest

class SavingAccountTest: XCTestCase {

    func testDeposit() {
        var account = SavingAccount()
        account.deposit(100)
        XCTAssertEqual(100, account.balance)
    }
}

Run All Tests

Run all the unit tests, it fails as we expected.

Write Code to Pass the Test

Create a Swift file named SavingAccount, and choose both BankAccount and BankAccountTests as targets.

Make it simple, just to pass the test.

1
2
3
4
5
6
7
8
9
import Foundation

class SavingAccount {
    var balance:Int = 100

    func deposit(money:Int) {

    }
}

Run All Tests

It passes.

Next User Story?

“People could withdraw some money”

Let’s change the testDeposit test case.

1
2
3
4
5
6
7
8
9
10
11
12
import Foundation
import XCTest

class SavingAccountTest: XCTestCase {

    func testDepositAndWithdraw() {
        var account = SavingAccount()
        account.deposit(100)
        account.withdraw(50)
        XCTAssertEqual(50, account.balance)
    }
}

Also, add an empty withdraw method to SavingAccount to satisfy the compiler. Do not add any other code until we see it fails.

Run All Tests

The test fails, because the account balance was not updated after people withdrew some money.

Write Code to Support Withdraw

1
2
3
4
5
6
7
8
9
10
11
12
13
import Foundation

class SavingAccount {
    var balance:Int = 0

    func deposit(money:Int) {
        balance += money
    }

    func withdraw(money:Int) {
        balance -= money
    }
}

Run All Tests

All the user stories are satisfied.

Any Other New User Story?

“People can’t withdraw money beyond their account balance”

We add a new test case testNegativeBalanceIsNotFine

1
2
3
4
5
6
func testNegativeBalanceIsNotFine() {
    var account = SavingAccount()
    account.deposit(50)
    account.withdraw(100)
    XCTAssertEqual(0, account.balance)
}

Run All Tests

It fails, we have to fix it.

Write Code

Change the withdraw method, set account balance to 0 if it is less than 0.

1
2
3
4
5
6
func withdraw(money:Int) {
    balance -= money
    if balance < 0 {
        balance = 0
    }
}

Run All Tests

All right, all the test cases are succeeded.

Refactoring

Until now, we haven’t do any refactoring on our code base.

I think the production code is fine, so we skip the step 5, and refactor the test code.

We can see that both test cases create an instance of SavingAccount, the duplicated code can be removed by using only one SavingAccount instance.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
class SavingAccountTest: XCTestCase {
    var account = SavingAccount()

    func testDepositAndWithdraw() {
        account.deposit(100)
        account.withdraw(50)
        XCTAssertEqual(50, account.balance)
    }

    func testNegativeBalanceIsNotFine() {
        account.deposit(50)
        account.withdraw(100)
        XCTAssertEqual(0, account.balance)
    }
}

Don’t forget to run all the tests, make sure it is succeeded.

Why no setup and tearDown

People coming from objc may doubt that why the account instance is not put into setUp method, the way we use might cause different test cases sharing one instance variable, as we know, test cases should be independent with each other.

Yes, I had this doubt, too. So I did a test, by adding a “account balance should be 0” check before each test cases.

1
2
3
4
5
6
7
8
9
10
11
12
13
func testDepositAndWithdraw() {
    XCTAssertEqual(0, account.balance)
    account.deposit(100)
    account.withdraw(50)
    XCTAssertEqual(50, account.balance)
}

func testNegativeBalanceIsNotFine() {
    XCTAssertEqual(0, account.balance)
    account.deposit(50)
    account.withdraw(100)
    XCTAssertEqual(0, account.balance)
}

The result shows that the XCTest framework avoids instance variable sharing between test cases by instantiating a brand new XCTestCase object for each test case. That is, it instantiated two SavingAccountTest objects as our tests run.

To TDD Haters

If you hate TDD, and may think this blog post is garbage.

Sorry for that, you can remove your browser history of this address, if it makes you feel better.

Also, I strongly recommend you to watch the “TDD dead” discussions by Martin Fowler, Kent Beck and David Heinemeier Hansson.