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

Unix sockets raise exception on long writes #3799

Closed
evanphx opened this Issue Apr 12, 2016 · 21 comments

Comments

Projects
None yet
8 participants
@evanphx

evanphx commented Apr 12, 2016

Environment

  • jruby 9.0.5.0 (2.2.3) 2016-01-26 7bee00d Java HotSpot(TM) 64-Bit Server VM 25.72-b15 on 1.8.0_72-b15 +jit [darwin-x86_64]
  • Darwin veritas.local 15.4.0 Darwin Kernel Version 15.4.0: Fri Feb 26 22:08:05 PST 2016; root:xnu-3248.40.184~3/RELEASE_X86_64 x86_64
  • Rails app running puma: https://github.com/cdonadeo/puma_big_response_test

Expected Behavior

Long strings passed to UNIXSocket#write writes the data and returns the number of bytes written.

Actual Behavior

#write writes a subset of the data and raises an exception: #<SystemCallError: Unknown error - >}. The amount of data varies but I've seen 8192 show up not-randomly, almost like there was a 8k buffer somewhere.

The git repo linked to above shows the behavior. The exception in question is raise inside #fast_write. Changing the code to use #write instead of #syswrite does not change the behavior.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Apr 12, 2016

Member

Nice, haven't had a good UNIXSocket bug in a while. What are UNIX sockets used for in a puma set up?

I've got some long flights today so I may have time to look into this.

Member

headius commented Apr 12, 2016

Nice, haven't had a good UNIXSocket bug in a while. What are UNIX sockets used for in a puma set up?

I've got some long flights today so I may have time to look into this.

@TheKidCoder

This comment has been minimized.

Show comment
Hide comment
@TheKidCoder

TheKidCoder Apr 12, 2016

Puma can use a UNIX socket to listen/respond to HTTP requests. For example, you can proxy requests from Nginx to Puma over a UNIX socket.

TheKidCoder commented Apr 12, 2016

Puma can use a UNIX socket to listen/respond to HTTP requests. For example, you can proxy requests from Nginx to Puma over a UNIX socket.

@evanphx

This comment has been minimized.

Show comment
Hide comment

evanphx commented Apr 12, 2016

@headius What @TheKidCoder said.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Apr 12, 2016

Member

Ok that makes sense.

Member

headius commented Apr 12, 2016

Ok that makes sense.

@ericqweinstein

This comment has been minimized.

Show comment
Hide comment
@ericqweinstein

ericqweinstein Apr 13, 2016

Does this issue correspond to this constant?

ericqweinstein commented Apr 13, 2016

Does this issue correspond to this constant?

@headius headius added this to the JRuby 9.1.1.0 milestone Apr 20, 2016

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Apr 20, 2016

Member

@ericqweinstein Seems plausible...I'm not familiar with that value.

Member

headius commented Apr 20, 2016

@ericqweinstein Seems plausible...I'm not familiar with that value.

@headius headius modified the milestones: JRuby 9.1.1.0, JRuby 9.1.2.0 May 11, 2016

@enebo enebo modified the milestones: JRuby 9.1.2.0, JRuby 9.1.3.0 May 23, 2016

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Aug 22, 2016

Member

We may be able to make progress on this with @etehtsea helping work on sockets. Sadly it won't be in 9.1.3.0.

Member

headius commented Aug 22, 2016

We may be able to make progress on this with @etehtsea helping work on sockets. Sadly it won't be in 9.1.3.0.

@headius headius modified the milestones: JRuby 9.1.4.0, JRuby 9.1.3.0 Aug 22, 2016

@etehtsea

This comment has been minimized.

Show comment
Hide comment
@etehtsea

etehtsea Aug 31, 2016

Contributor

Hopefully, I have a fix for this.

Contributor

etehtsea commented Aug 31, 2016

Hopefully, I have a fix for this.

etehtsea added a commit to etehtsea/jruby that referenced this issue Aug 31, 2016

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Nov 8, 2016

Member

@etehtsea Got that fix in a releasable state? We're hoping to push 9.1.6.0 tomorrow.

Member

headius commented Nov 8, 2016

@etehtsea Got that fix in a releasable state? We're hoping to push 9.1.6.0 tomorrow.

@etehtsea

This comment has been minimized.

Show comment
Hide comment
@etehtsea

etehtsea Nov 8, 2016

Contributor

@headius no, I don't have better solution for now.

Contributor

etehtsea commented Nov 8, 2016

@headius no, I don't have better solution for now.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Nov 8, 2016

Member

@etehtsea I guess what I was asking is if the commit above is a "good enough" fix. If so, do you want to throw it in a PR for testing?

Status of testing is unclear and this is a key core API. Punting.

Member

headius commented Nov 8, 2016

@etehtsea I guess what I was asking is if the commit above is a "good enough" fix. If so, do you want to throw it in a PR for testing?

Status of testing is unclear and this is a key core API. Punting.

@headius headius modified the milestones: JRuby 9.1.7.0, JRuby 9.1.6.0 Nov 8, 2016

@etehtsea

This comment has been minimized.

Show comment
Hide comment
@etehtsea

etehtsea Nov 9, 2016

Contributor

@headius you said earlier that this fix couldn't be "good enough" because possibly it masks real issue.

As far as I remember I tested this issue using this app so if needed I could try to convert it to regression spec.

Contributor

etehtsea commented Nov 9, 2016

@headius you said earlier that this fix couldn't be "good enough" because possibly it masks real issue.

As far as I remember I tested this issue using this app so if needed I could try to convert it to regression spec.

etehtsea added a commit to etehtsea/jruby that referenced this issue Nov 16, 2016

@headius headius closed this in bfcec47 Nov 22, 2016

headius added a commit that referenced this issue Nov 22, 2016

@sumitmah

This comment has been minimized.

Show comment
Hide comment
@sumitmah

sumitmah Jan 12, 2017

@headius I'm still not getting consistent behaviour with 9.1.7.0 for the puma_big_response_test example app created by @evanphx.

sumitmah commented Jan 12, 2017

@headius I'm still not getting consistent behaviour with 9.1.7.0 for the puma_big_response_test example app created by @evanphx.

@sumitmah

This comment has been minimized.

Show comment
Hide comment
@sumitmah

sumitmah Jan 13, 2017

@headius we never merged following commit to master. etehtsea@613c99f

sumitmah commented Jan 13, 2017

@headius we never merged following commit to master. etehtsea@613c99f

@mavenik

This comment has been minimized.

Show comment
Hide comment
@mavenik

mavenik Jan 30, 2017

@headius This is still happening on 9.1.7.0 for long writes and Unix sockets with Puma. Switching to TCP port configuration for Puma has resolved it temporarily, but that's not a viable solution IMO.

Any ETA on the fix?

mavenik commented Jan 30, 2017

@headius This is still happening on 9.1.7.0 for long writes and Unix sockets with Puma. Switching to TCP port configuration for Puma has resolved it temporarily, but that's not a viable solution IMO.

Any ETA on the fix?

@etehtsea

This comment has been minimized.

Show comment
Hide comment
@etehtsea

etehtsea Jan 30, 2017

Contributor

@mavenik @sumitmah on what OS you caught this?

Contributor

etehtsea commented Jan 30, 2017

@mavenik @sumitmah on what OS you caught this?

@sumitmah

This comment has been minimized.

Show comment
Hide comment
@sumitmah

sumitmah Jan 30, 2017

@etehtsea I can reproduce on linux. This commit etehtsea@613c99f I've tested, supposed to fix it but it never got merged to master. Only next commit after this was merged.

sumitmah commented Jan 30, 2017

@etehtsea I can reproduce on linux. This commit etehtsea@613c99f I've tested, supposed to fix it but it never got merged to master. Only next commit after this was merged.

@etehtsea

This comment has been minimized.

Show comment
Hide comment
@etehtsea

etehtsea Jan 30, 2017

Contributor

@sumitmah @mavenik At some point I decided that I had gone crazy, but actually this issue was fixed after updating to jnr-enxio 0.14 and appeared again after updating to jnr-unixsocket 0.15.
It work properly with jnix-unixsocket 0.14. I'm looking for the fix now.

cc: @enebo @headius

Contributor

etehtsea commented Jan 30, 2017

@sumitmah @mavenik At some point I decided that I had gone crazy, but actually this issue was fixed after updating to jnr-enxio 0.14 and appeared again after updating to jnr-unixsocket 0.15.
It work properly with jnix-unixsocket 0.14. I'm looking for the fix now.

cc: @enebo @headius

etehtsea added a commit to etehtsea/jruby that referenced this issue Jan 30, 2017

Fix #3799 again.
Fixed code from jnr-enxio is not used anymore after updating to
jnr-unixsocket 0.15.

etehtsea added a commit to etehtsea/jruby that referenced this issue Jan 30, 2017

Fix #3799 again.
See jnr/jnr-unixsocket#38
Fixed code from jnr-enxio is not used anymore after updating to
jnr-unixsocket 0.15.
Regression spec accidentally passes on TravisCI and Ubuntu xenial.
In the meantime there are bug reports that issue still
reproduces in some environments. Personally I caught it under macOS.
@etehtsea

This comment has been minimized.

Show comment
Hide comment
@etehtsea

etehtsea Jan 30, 2017

Contributor

@sumitmah @mavenik should be fixed when #4466 will be merged.

Contributor

etehtsea commented Jan 30, 2017

@sumitmah @mavenik should be fixed when #4466 will be merged.

headius added a commit that referenced this issue Feb 9, 2017

@etehtsea

This comment has been minimized.

Show comment
Hide comment
@etehtsea

etehtsea Feb 12, 2017

Contributor

@sumitmah @mavenik please check again on master

Contributor

etehtsea commented Feb 12, 2017

@sumitmah @mavenik please check again on master

@mavenik

This comment has been minimized.

Show comment
Hide comment
@mavenik

mavenik Feb 13, 2017

@etehtsea Thanks! Will fetch master and verify this.

mavenik commented Feb 13, 2017

@etehtsea Thanks! Will fetch master and verify this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment