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

Allow setting stream chunk size in SocketHandler #1129

Merged
merged 4 commits into from Jun 18, 2018
Merged

Conversation

klemenb
Copy link
Contributor

@klemenb klemenb commented Mar 28, 2018

This pull requests adds an option to the SocketHandler to set the desired stream chunk size via stream_set_chunk_size.

The main problem this pull request solves is logging larger messages via UDP using SocketHandler. The default chunk size set by PHP appears to be 8192 and any larger UDP messages get split into multiple packets and result in multiple log entries. When used with logstash (and similar processors) those messages are therefore not parsed correctly.

Since the UDP protocol allows for much larger packets (https://en.wikipedia.org/wiki/User_Datagram_Protocol#Packet_structure), this problem can be solved by simply using a larger chunk size.

@Seldaek
Copy link
Owner

Seldaek commented Jun 7, 2018

Thanks, I wonder why not just set it to a larger size though if it's overall a good thing? Is there a case where we don't want it to be long? 65,507 seems to be the max, I don't know if we should default to that or something more conservative but it sounds to me like having a smarter default beats one more option that people have to even know about to make use of.

@Seldaek Seldaek added the Bug label Jun 7, 2018
@klemenb
Copy link
Contributor Author

klemenb commented Jun 7, 2018

Setting chunk size affects TCP streams as well and it could cause some unexpected issues in certain environments.

Also, one reservation that is more or less moot even at 8192 is that large UDP packets are even more unreliable - https://stackoverflow.com/a/35697810.

Honestly, I currently cannot think of a specific scenario where a larger default chunk size would cause issues, but my gut feeling says it is a bad idea to silently change this.

@Seldaek
Copy link
Owner

Seldaek commented Jun 8, 2018

Alright makes sense. One more thing though in that case, I'd rather have this default to null and only do the stream_set_chunk_size call in case a chunk size was provided, so if users don't provide anything the default is used whatever it is. In case the default value changes in PHP it's probably best to avoid overriding it with our own default.

@Seldaek Seldaek added this to the 1.x milestone Jun 8, 2018
@Seldaek Seldaek merged commit e8db808 into Seldaek:1.x Jun 18, 2018
@Seldaek
Copy link
Owner

Seldaek commented Jun 18, 2018

Thanks, I did the changes myself.

@klemenb
Copy link
Contributor Author

klemenb commented Jun 18, 2018

Great, thanks. Sorry I wasn't available in the past few days.

@Seldaek
Copy link
Owner

Seldaek commented Jun 18, 2018

No worries, just trying to move things forward.

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.

None yet

2 participants