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.

Sign up for GitHub

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

DIRMINA-1173 #44

Merged
merged 25 commits into from
Sep 11, 2024
Merged

DIRMINA-1173 #44

Changes from 1 commit
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
0cbbc8b
working on the multi-phase synchronization; it was working before but…
jon-valliere Feb 20, 2024
ec93bb7
nonblock SSL now passes all tests
jon-valliere Feb 20, 2024
d393ad7
found one place where the UNACK was calculated wrong
jon-valliere Feb 20, 2024
1a4fbda
typos
jon-valliere Feb 22, 2024
05028e8
found an issue where async task execution was always happening inline
jon-valliere Feb 22, 2024
f5f70ed
add accidentally removed sync
jon-valliere Feb 22, 2024
93a4ff7
small change to the control flow for public methods
jon-valliere Feb 24, 2024
ed97be8
adds separate event queue - this is temporary because I don't like th…
jon-valliere Feb 29, 2024
62643fe
alright lets remove all the synchronization from the queue flushes
jon-valliere Mar 1, 2024
df98d09
Free a useless buffer.
elecharny Apr 18, 2024
46fb20f
Merge branch '2.2.X' into bugfix/DIRMINA-1173
elecharny Apr 18, 2024
13d5019
removed a useless import
elecharny Apr 26, 2024
cc86146
Used an AtomicBooleaninstead of using a lock
elecharny Apr 26, 2024
57767e4
Added some new lines for clarity
elecharny Apr 26, 2024
df0ecc6
Disable Nagle's algorithm by default
elecharny May 11, 2024
495a282
Revert the change made on the TCP_NODELAY default value.
elecharny May 11, 2024
0daeb7b
Upgraded PMD to 7.0.0 and pmd-pligin to 3.22
elecharny May 27, 2024
6ab8003
Give more significant name to a variable
elecharny Sep 10, 2024
e3dc20b
Switched to Easymock 5.4.0 as a replacemnt for rmock, hich does not
elecharny Sep 10, 2024
ac9ce3f
Bumped up ognl dependency and PMD
elecharny Sep 10, 2024
2904be1
Bumped up some maven dependencies
elecharny Sep 10, 2024
dd403eb
Bumped up maven dependency versions
elecharny Sep 10, 2024
0fc698a
Added a JenkinsFile
elecharny Sep 11, 2024
994a00b
Deleted badly named Jenkinsfile
elecharny Sep 11, 2024
cf86678
Renamed the Jenkinsfile
elecharny Sep 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion mina-core/src/main/java/org/apache/mina/transport/socket/DefaultSocketSessionConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public class DefaultSocketSessionConfig extends AbstractSocketSessionConfig {

private static final int DEFAULT_SO_LINGER = -1;

private static final boolean DEFAULT_TCP_NO_DELAY = false;
private static final boolean DEFAULT_TCP_NO_DELAY = true; // Disable Nagle algorithm by default
Copy link
Author

@jon-valliere jon-valliere May 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hey @elecharny I'm not sure this is a good idea. If we want todo this also, I think it should be as part of a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @jon-valliere, just read this blog post: https://brooker.co.za/blog/2024/05/09/nagle.html

Somehow, Nagle's algorithm is a thing of the past (ie when we were dealing with terminals over slow connections). In MINA's context, and if we except SSHD, I do think that the default to disable Nagle's algorithm actually makes sense. Most of MINA based application don't deal with char by char communication, rather with quite big chunks of data, so there is little need to wait for an ACK from the server.

Anyway, this is configurable, so I guess it should not be a big deal.

Copy link
Author

Choose a reason for hiding this comment

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

Disabling Nagle by default may create context switching performance hits in certain applications. Any situation where you may invoke more than one write in sequence, receives performance boosts from the way the IO scheduler works in TCP because it can produce MORE packets in a single go and does not have to be woken up as often. The OS itself does not disable Nagle by default and I do not think we should make this decision for the end user. As long as they can configure it, I think the default should be unset (to allow the OS default to apply). If other applications, built on MINA, want to configure a new default then it is up to them. For SSHD or Apache Directory, if you want to disable Nagle there, that is fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not arguing here, I will revert but...

I do think that this flag should be opt-in, ie the Nagle algorithm should be disabled by default. The use cases were it's useful are not really the most frequent, and the drawbacks are numerous. If the default sendBuffer size is set to 64Ko, then you have a delayed for every packet that has a lower number of bytes to transfert. if you set the sendBuffer size to a lower value, then you are sending way more packet than needed. In any case, if you are to send some data that span on more than one packet, and the last packet is not completed, then you'll pay a 200ms delay every time. Not very cool.

With Gigabits connections, sending a lot of packets is not really an issue anymore, except on heavily loaded applications that aren't interactive (HTTP may enter into this category, but with REST application, I would argue that an immediate sending is a plus).

Regarding LDAP, requests are small there is no interest in differing the packet sendings, so yes, I'm likely to deactivate the Nagle Algorithm.

Seems like on Linux they have added the TCP_CORK parameter to let the sender decide when to send the data, bypassing the Nagle's algorithm completely, and also solving the context switching pb at the same time (somehow it's a way to say "you're responsability, not teh OS one", and I tend to agree). Sadly, this is not portable, so currently it's not an option.

Last, not least, for TLS HS, Nagle's Algorithm is delaying the whole negociation, which is a real PITA...

Copy link
Author

Choose a reason for hiding this comment

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

In my other IO framework, all of the TCP/SOCK options all have unset states where the framework code will not call the SOCK configuration at all and allow the OS defaults to be applied.

Copy link
Author

Choose a reason for hiding this comment

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

See https://en.wikipedia.org/wiki/Nagle%27s_algorithm

Nagle only really applies for when the packets are smaller than the MSS which is usually 1280 bytes. AFAIK only the HELLO message in TLS HS is smaller than this; all other messages SHOULD be aggregated together by the SSL layer. There IS a well known interaction with Nagle and Delayed Ack which can cause Nagle to be worse than it would be by itself. What is the order of delays you are seeing?

Copy link
Contributor

Choose a reason for hiding this comment

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

I missed the MSS part. Ok, so makes sense to keep the parameter as is, but I do agree that not setting a default at all would be a better option. At least, you let the developer makes a choice depending on what they actually measure.

Thanks Jon!


protected IoService parent;

Expand Down
Loading