Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 341171

Summary:

Locking in HttpDestination blocks all requests to the same address

Product:

[RT] Jetty

Reporter:

Chris Dumoulin <chris>

Component:

client

Assignee:

Simone Bordet <simone.bordet>

Status:

RESOLVED FIXED

QA Contact:

Severity:

normal

Priority:

CC:

gregw, jetty-inbox

Version:

unspecified

Target Milestone:

7.2.x

Hardware:

OS:

Linux

Whiteboard:

Attachments:

Description	Flags
Patch for a proposed fix.	none
Different diff format for proposed patch	simone.bordet: iplog+

Description Chris Dumoulin

2011-03-28 17:23:28 EDT

Build Identifier: Jetty distribution 7.2.0.v20101020

In org.eclipse.jetty.client.HttpDestination calls to startNewConnection() are being made from within synchronized blocks. During the startNewConnection() call, all HttpConnections for that HttpDestination are blocked from being returned to the HttpDestination and calls to HttpDestination.getIdleConnection() are also blocked.

Since creating a new socket connection can be very slow and startNewConnection() doesn't need to be called from within a synchronized block, calls to startNewConnection() should be moved outside of synchronized blocks.

This is safe because:
- startNewConnection itself performs necessary synchronization to update _pendingConnections.
- The call to connector.startConnection() doesn't need to be synchronized.
- The call to connector.startConnection() will result in one of: onConnectionFailed(), onException(), or onNewConnection() being called. Each of which handles necessary synchronization.

I'll be attaching a patch with a proposed fix.

Reproducible: Always

Steps to Reproduce:
1. Make multiple simultaneous requests to the same web server.
2. Force the server to be slow in accepting one of the connections.
3. Observe that getIdleConnection() and returnConnection() will both block while the slow connection is being made.

Comment 1 Chris Dumoulin

2011-03-28 17:27:34 EDT

Created attachment 192045 [details]
Patch for a proposed fix.

Comment 2 Chris Dumoulin

2011-03-28 17:36:55 EDT

Created attachment 192046 [details]
Different diff format for proposed patch

I'm not sure about the diff format of my initial patch file (what's up with the exclamation marks). I created it using "diff -crB" based on some website's advice.

This new file is from an svn diff of the change I'm proposing (I've got a copy of the jetty source in a local subversion repo). If neither of these versions of the patch are acceptable, please let me know the proper way to provide a patch.

Comment 3 Greg Wilkins

2011-04-01 00:05:25 EDT

Simone,

can you review the state of this issue.  It may be that the changes I made to revert to an async connect may prevent the blocking anyway?

Comment 4 Simone Bordet

2011-04-04 09:49:53 EDT

We have enhanced the connect functionality to support blocking style and non blocking style with the SelectConnector, and the default is blocking.
The SocketConnector can only do blocking connects.

So I am reviewing the patch as we should really attempt to connect outside sync blocks.

Comment 5 Simone Bordet

2011-04-04 10:30:05 EDT

Patch applied with small changes only to variable names.