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

Emmit small socket datas #289

Closed
wants to merge 3 commits into from
Closed

Emmit small socket datas #289

wants to merge 3 commits into from

Conversation

artggd
Copy link
Contributor

@artggd artggd commented Apr 7, 2014

fix #288
data test updated -> handleData($stream)

data test updated -> handleData($stream)
@artggd artggd changed the title fixes #288 Emmit small socket datas Apr 7, 2014
@@ -1,5 +1,5 @@
{
"name": "react/react",
"name": "artydev/react",
Copy link

Choose a reason for hiding this comment

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

unrelated change

@cboden
Copy link
Member

cboden commented Apr 7, 2014

Can you add a unit test that demonstrates this fix and fails on the existing code please?

@artggd
Copy link
Contributor Author

artggd commented Apr 11, 2014

So, after a long debugging phase, I found out the problem occurs when sending data from a socket in python language. I don't really know where is the difference between a socket in php and in python, Even after analysing the packets, I can't see any differences ...
Anyway, I pushed units tests for python on my fork (master). It works with my modification and fails with the existing code.

@cboden
Copy link
Member

cboden commented Apr 13, 2014

Excellent test case! Here's what I think is happening:

  1. Python queues data to be sent to the server
    1.1) When you send >4096 (or w/e the limit is) the write buffer is flushed and data sent throughout the pipe - all is well, stop here
    1.2) Sending a small amount TCP, as per its specification, chooses not to send it yet
  2. Python sends the shutdown signal that it's finished writing, but remains open for listening
  3. The write buffer and shutdown signal are sent over the wire
  4. React receives that data in its read buffer as well as the write shutdown signal
  5. PHP streams, specifically feof, can't detect the difference between shutdown signals and sees the whole stream as closed and emits an end() event

Your fix is good, it will check the read buffer and then check if it was closes. I'm going to attempt to re-work the unit test to use PHP and then merge it.

@cboden cboden added this to the v0.4.1 milestone Apr 13, 2014
@cboden cboden added stream and removed bug labels Apr 13, 2014
@cboden cboden self-assigned this Apr 13, 2014
@artggd
Copy link
Contributor Author

artggd commented Apr 13, 2014

It's probably something like that, maybe a deeper analysis of the packets could confirm.
Glad to hear my small fix helped ! ;)

@cboden
Copy link
Member

cboden commented Apr 13, 2014

I ended up cherry picking (I know, scold me) your changes into the 0.4 branch, replacing with a phpunit test, tagged v0.4.1, and merged back into master.

Thanks @artydev!

@cboden cboden closed this Apr 13, 2014
@cboden
Copy link
Member

cboden commented Apr 14, 2014

Just for reference I also made a slight fix: ebf704e#diff-77e069da74242a16d316721cb77c42beR14

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

Successfully merging this pull request may close these issues.

Socket datas never handled if too small
3 participants