ByteBuffer corrupted by ByteBufferConverter when passed through Spring [SPR-13056]

[spring-projects-issues]

Nathan Hull opened SPR-13056 and commented

It looks like spring-core 4.x has a bug in how it handles a ByteBuffer being sent on a Spring channel. It converts the ByteBuffer to a byte array, then takes the first element of that array, converts that to a byte array, and then wraps that byte array in a new ByteBuffer. As a result, when I pass a ByteBuffer on a Spring channel, I get a different ByteBuffer out and it only contains the first element, if the ByteBuffer had one or more elements remaining. Most of this happens in ByteBufferConverter.

---

Attachments:

• springBug-sources.jar (2.16 kB)

Referenced from: commits 792b7b9, 1177f5c, 008c9a3, fee63fd

[spring-projects-issues]

Juergen Hoeller commented

This doesn't seem to be a bug in ByteBufferConverter itself since a regular conversion attempt from ByteBuffer to ByteBuffer works fine, delivering a fresh copy of that buffer.

I assume you're experiencing this issue through Spring Integration? If so, I'd suggest raising it with those guys on their JIRA, since the extraction of a single element probably happens somewhere in the specific conversion handling. If the root of the problem turns out to be in the core ConversionService, let me know and I'll revisit it here. However, please provide a test case for it at that point.

Juergen

[spring-projects-issues]

Nathan Hull commented

I just attached sources for a dummy project that demonstrates the bug. All it does is send a ByteBuffer via a gateway to a service activator. I don't know how to tell if it's a problem in core or integration.

[spring-projects-issues]

Juergen Hoeller commented

I've moved this over to Spring Integration since that's the better starting point for an analysis here...

Juergen

[spring-projects-issues]

Artem Bilan commented

Juergen Hoeller, I've just done this test:

public class Int3720Tests {

	@Test
	public void testIt() {
		TypeDescriptor typeDescriptor = TypeDescriptor.valueOf(ByteBuffer.class);

		ByteBuffer byteBuffer = ByteBuffer.allocate(2);
		byteBuffer.put((byte) 1);
		byteBuffer.put((byte) 2);

		byteBuffer.rewind();

		ConfigurableApplicationContext context = new AnnotationConfigApplicationContext(Context.class);
		ConversionService conversionService = context.getBean(ConversionService.class);
		Object result = conversionService.convert(byteBuffer, typeDescriptor, typeDescriptor);

		assertEquals(byteBuffer, result);

		byteBuffer.rewind();

		ConfigurableApplicationContext intContext = new AnnotationConfigApplicationContext(IntegrationContext.class);
		ConversionService intConversionService = intContext.getBean(ConversionService.class);
		result = intConversionService.convert(byteBuffer, typeDescriptor, typeDescriptor);

		assertEquals(byteBuffer, result);

		context.close();
		intContext.close();
	}

	@Configuration
	@EnableIntegration
	public static class IntegrationContext {

	}

	@Configuration
	public static class Context {

		@Bean
		public FactoryBean<ConversionService> conversionService() {
			return new ConversionServiceFactoryBean();
		}

	}

}

And it really shows me that ByteBufferConverter doesn't convert properly ByteBuffer to ByteBuffer.
We fail here on the raw @Configuration without Spring Integration.

From my perspective we must not even try to convert if the target is ByteBuffer.

Would you mind sharing your test-case to take a look ?

Thanks!

[spring-projects-issues]

Juergen Hoeller commented

Artem Bilan, I was just using this one:

@Test
public void byteBufferToByteBuffer() throws Exception {
     byte[] bytes = new byte[] { 1, 2, 3 };
     ByteBuffer byteBuffer = ByteBuffer.wrap(bytes);
     ByteBuffer convert = this.conversionService.convert(byteBuffer, ByteBuffer.class);
     assertThat(convert, not(sameInstance(byteBuffer.rewind())));
     assertThat(convert, equalTo(byteBuffer.rewind()));
}

Looking at your test case, you seem to reuse the original ByteBuffer instance? That might be the root of the problem here: It's a stateful buffer object after all, with all that rewind() business, it's even somewhat odd to write a test case for it. So if there is an easy improvement that we can make to ByteBufferConverter itself for such scenarios, I'm up for that. However, generally speaking, you shouldn't pass a previously consumed buffer/stream/etc to the ConversionService again. I'd suggest to either create fresh a ByteBuffer instance every time or to specifically skip the ConversionService invocation for such reuse scenarios.

[spring-projects-issues]

Artem Bilan commented

Thank you, Juergen, but doesn't work even like this:

	byte[] bytes = new byte[] { 1, 2, 3 };

	ByteBuffer byteBuffer = ByteBuffer.wrap(bytes);

	ConfigurableApplicationContext context = new AnnotationConfigApplicationContext(Context.class);
	ConversionService conversionService = context.getBean(ConversionService.class);
	ByteBuffer result = conversionService.convert(byteBuffer, ByteBuffer.class);

	assertThat(result, equalTo(ByteBuffer.wrap(bytes)));

java.lang.AssertionError: 
Expected: <java.nio.HeapByteBuffer[pos=0 lim=3 cap=3]>
     but: was <java.nio.HeapByteBuffer[pos=0 lim=1 cap=1]>

We can't bypass ConversionService, becuase SI POJO method invocation is based on SpEL, which uses ConversionService for method arguments from Message and its payload.
In this case our Message has payload as a ByteBuffer and the method param is ByteBuffer, too. So, or we should bypass conversion ByteBuffer -> ByteBuffer, or should copy it to a new one, as you said that we shouldn't reuse a consumed already.
But I don't see that it works properly in the ByteBufferConverter...

I am on the SF-4.2.0.BULD-SNAPSHOT.

Come back to you later, when I fix some my own issue and prepare a raw test-case just for ByteBufferConverter.

Thank you for your time anyway!

[spring-projects-issues]

Juergen Hoeller commented

I've found the key difference in the tests: Our ByteBufferConverterTests use a GenericConversionService with just ByteBufferConverter registered. When running them against a full DefaultConversionService, two of the existing tests fail, so it's probably some other default converter (the array converter possibly?) interfering.

I'll have a look at the DefaultConversionService scenario ASAP. Moving this issue back to the Spring Framework project for that reason.

Juergen

[spring-projects-issues]

Nathan Hull commented

The logic around the call to ByteBufferConverter#convertFromByteBuffer() looks suspect: the preceding logic (ByteBufferConverter#convert()) determines that a HeapByteBuffer is assignable to a ByteBuffer, but then it calls ByteBufferConverter#convertFromByteBuffer(), which creates a new ByteBuffer. Why not return the original HeapByteBuffer since it's assignable to ByteBuffer?

[spring-projects-issues]

Juergen Hoeller commented

We intentionally create an independent copy of the ByteBuffer there, since each buffer can have its own position state then. But point taken, an explicit code path that duplicates the ByteBuffer doesn't hurt, so I've added that now.

Generally speaking, the root of the problem was that ByteBufferConverter didn't explicitly declare ByteBuffer->byte[] and vice versa as convertible pairs. This allows for other converters, such as the array-to-element converter, to kick in. I've added explicit declarations for byte[] next to the general Object convertible declarations now, which is sufficient to solve this problem. The explicit duplication code path above is just an optimization.

Juergen

[spring-projects-issues]

Juergen Hoeller commented

This is now in master and will be available in the upcoming 4.2.0.BUILD-SNAPSHOT. I'll backport it to 4.1.7 later today.

Juergen

[spring-projects-issues]

Artem Bilan commented

Thanks, Juergen!

Works well now.