Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reusable Payload #209

Merged
merged 2 commits into from
Jan 9, 2017
Merged

Reusable Payload #209

merged 2 commits into from
Jan 9, 2017

Conversation

NiteshKant
Copy link
Contributor

Problem

PayloadImpl caches data and metadata buffers. This means once those buffers are read, they can not be reused as the pointer of ByteBuffer has moved to the end of buffer.
This restricts the usage of PayloadImpl instance with something like:

socket.requestResponse(new PayloadImpl("Hello")).retry(2);

For any retry in the above code, data read from the payload instance will be empty.

Modification

  • store buffer position() on creation and reset them on every get().
  • Additional constructor to override this behavior and create a payload for single use.
  • PayloadImpl defaults to reusable payload.

Result

Possible to retry a request without having a custom Payload implementation.

* @param singleUse {@code true} if the buffer position is to be reset on every invocation of {@link #getData()} and
* {@link #getMetadata()}.
*/
public PayloadImpl(ByteBuffer data, ByteBuffer metadata, boolean singleUse) {
this.data = data;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we record the current position of the data, and the metadata and return a slice from that position instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@robertroeser ByteBuffer.slice() always uses the current position i.e. there is no such method as ByteBuffer.slice(position). So, if we were to slice, we still have to reset the position first.
Do you see any other value in using slice() even though we have to reset position first?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@stevegury stevegury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks correct to me.

* @param singleUse {@code true} if the buffer position is to be reset on every invocation of {@link #getData()} and
* {@link #getMetadata()}.
*/
public PayloadImpl(ByteBuffer data, ByteBuffer metadata, boolean singleUse) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: but renaming/reversing singleUse to reusable may be easier to understand (especially if you don't read the doc).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stevegury done, renamed to reusable and changed implementation!

@yschimke yschimke changed the title Resuable Payload Reusable Payload Jan 5, 2017
@ktoso
Copy link
Contributor

ktoso commented Jan 9, 2017

Ping, any chance to get this in soon? :)

__Problem__

`PayloadImpl` caches `data` and `metadata` buffers. This means once those buffers are read, they can not be reused as the pointer of `ByteBuffer` has moved to the end of buffer.
This restricts the usage of `PayloadImpl` instance with something like:

```java
socket.requestResponse(new PayloadImpl("Hello")).retry(2);
```

For any retry in the above code, data read from the payload instance will be empty.

__Modification__

- store buffer `position()` on creation and reset them on every `get()`.
- Additional constructor to override this behavior and create a payload for single use.
- `PayloadImpl` defaults to reusable payload.

__Result__

Possible to retry a request without having a custom `Payload` implementation.
@NiteshKant NiteshKant merged commit e5dc7e0 into rsocket:0.5.x Jan 9, 2017
@NiteshKant
Copy link
Contributor Author

@ktoso you got it 😄

@NiteshKant NiteshKant deleted the 0.5.x-payload branch January 9, 2017 18:43
@ktoso
Copy link
Contributor

ktoso commented Jan 9, 2017

Excellent, thanks! Very nice to see much movement here in the new year 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants