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

support (seekable) ByteArrayInputStream#to_io.rewind #3337

Merged
merged 5 commits into from Oct 20, 2015

Conversation

Projects
None yet
2 participants
@kares
Member

kares commented Sep 17, 2015

except FileInputStream it seems quite useful to be able to rewind/seek a byte[] backed stream

this is not possible using Channels.newChannel as the returned channel (except for files) is not seekable
but using the 9K refactored RubyIO its fairly easy to play a trick by returning a custom channel instance ...

1.7.x remains behaving weirdly on rewind ... more notes at #2381

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Oct 14, 2015

Member

@kares I appreciate the attribution but the code at 6142ea3#diff-b60adbaec9c15033c4eb0231a09d211aR46 worries me. We can't incorporate code from GPL codebases like this, even if they're been massaged into place a little.

I'd suggest you look at the Android implementation of this logic, which is licensed Apache-2.0. We can incorporate them into a separate file with an Apache license header and it should be compatible with all three of our licenses.

Here's the Android 4.3 impl of Channels at equivalent read method: https://android.googlesource.com/platform/libcore/+/android-4.3_r1/luni/src/main/java/java/nio/channels/Channels.java#297

Member

headius commented Oct 14, 2015

@kares I appreciate the attribution but the code at 6142ea3#diff-b60adbaec9c15033c4eb0231a09d211aR46 worries me. We can't incorporate code from GPL codebases like this, even if they're been massaged into place a little.

I'd suggest you look at the Android implementation of this logic, which is licensed Apache-2.0. We can incorporate them into a separate file with an Apache license header and it should be compatible with all three of our licenses.

Here's the Android 4.3 impl of Channels at equivalent read method: https://android.googlesource.com/platform/libcore/+/android-4.3_r1/luni/src/main/java/java/nio/channels/Channels.java#297

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Oct 20, 2015

Member

@headius thanks, the problematic part is now re-implemented as suggested.
tests fail due missing jnr-posix SNAPSHOT (currently, there's no green travis-ci build to rebase this off).

Member

kares commented Oct 20, 2015

@headius thanks, the problematic part is now re-implemented as suggested.
tests fail due missing jnr-posix SNAPSHOT (currently, there's no green travis-ci build to rebase this off).

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Oct 20, 2015

Member

@kares I'll pull it down today, review and make sure it's building, and push it out.

Member

headius commented Oct 20, 2015

@kares I'll pull it down today, review and make sure it's building, and push it out.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Oct 20, 2015

Member

Testing seems fine, I'll push shortly.

Last suggestions, if you get a chance (just throw them on master):

  • Try to grab and setAccessible the fields you access during static initialization; if we can't do it, we need to fall back to the unseekable wrapper.
  • Saving the fields will also be better perf rather than looking up and hitting setAccessible every time they're accessed.
Member

headius commented Oct 20, 2015

Testing seems fine, I'll push shortly.

Last suggestions, if you get a chance (just throw them on master):

  • Try to grab and setAccessible the fields you access during static initialization; if we can't do it, we need to fall back to the unseekable wrapper.
  • Saving the fields will also be better perf rather than looking up and hitting setAccessible every time they're accessed.

@headius headius merged commit e352864 into master Oct 20, 2015

@headius headius deleted the test-io-rewind branch Oct 20, 2015

kares added a commit to kares/jruby that referenced this pull request Oct 21, 2015

tune seekable byte[] channel impl to fallback in limited envs (as sug…
…gested in #3337)

... also keep the fields needed to avoid constant lookup in ByteArrayInputStream.class
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment