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

Jump to bottom

Do not lock while writing to a socket #52

Merged
merged 1 commit into from Oct 19, 2017
Merged

Do not lock while writing to a socket #52

merged 1 commit into from Oct 19, 2017

Conversation

idy
Copy link

@idy idy commented Oct 14, 2017 •
edited by domodwyer
Loading

A net.Conn is defined as being safe for concurrent access (here) however mgo holds the socket lock for the duration of a write unnecessarily.

@idy
Copy link
Author

idy commented Oct 14, 2017

fix #51

@domodwyer
Copy link

Hi @idy

First of all: thanks for taking the time to diagnose and open a PR! It's genuinely appreciated.

Just to double check, are you running this in production? It seems updateDeadline() is racy without being protected by the mutex, but I probably not in a "bad" way that would cause issues.

It's probably worth revisiting the locking in mgo at some point - it seems quite excessive and would be an easy performance gain.

Dom

@domodwyer domodwyer changed the title do not lock while writing to a socket Do not lock while writing to a socket Oct 16, 2017
@idy
Copy link
Author

idy commented Oct 19, 2017

TcpConn.SetDeadline do not need a mux to lock it, as it use atomic operation.

updateDeadline() only set some read/write deadlines to conn.

@idy
Copy link
Author

idy commented Oct 19, 2017

The read loop function has some operations require the lock, but the lock was locked while write to socket. This will stop the program from process the received respones from mongo.

Mongo will queue up requests, if response do not be consumed by program.

So the progrom hangs up. Program keep write requests to mongodb, and mongodb do not process it at all, because the program was locked to handle the mongodb's response, and it stops.

@idy
Copy link
Author

idy commented Oct 19, 2017

I confirmed this issue with 20000 ops/s, I test my code with go test options -race. After my patch, the program works fine, but without my patch, the program hangs up forever.

@idy
Copy link
Author

idy commented Oct 19, 2017

FYI, the socket is safe with multiple threads: https://golang.org/pkg/net/#Conn

Multiple goroutines may invoke methods on a Conn simultaneously.

@domodwyer
Copy link

Hi @idy

I was referring to calculating when concurrently and calling conn.SetDeadline(when) is racy - but as I said, it's unlikely to be a problem as the deadline set is a absolute time, not a relative addition. Concurrently set deadlines are unlikely to vary by any significant amount.

I've tested this in our staging environment and it definitively improves load handling - great spot!

Revisiting locking in mgo is on my TODO list, so thanks for helping, it's really appreciated! 👍

Dom

@domodwyer domodwyer merged commit 95448ed into globalsign:master Oct 19, 2017
@domodwyer domodwyer mentioned this pull request Oct 19, 2017
Merged
domodwyer added a commit that referenced this pull request Oct 19, 2017
@domodwyer
Revert "do not lock while writing to a socket (#52)"
8d9e210 
This reverts commit 95448ed.
domodwyer pushed a commit that referenced this pull request Oct 19, 2017
@idy @domodwyer
do not lock while writing to a socket (#52)
6adbadf 
fix #51
@domodwyer domodwyer mentioned this pull request Oct 19, 2017
Merged
@idy
Copy link
Author

idy commented Oct 19, 2017

Thanks to maitain this repo, god bless ;)

domodwyer added a commit that referenced this pull request Oct 19, 2017
@domodwyer
do not lock while writing to a socket (#52) (#54)
0454966 
fix #51
domodwyer added a commit that referenced this pull request Oct 22, 2017
@domodwyer
Merge commit '0454966c021aa1737c9680aceef7e2ba26952fe3' into merge
e10a339 
* commit '0454966c021aa1737c9680aceef7e2ba26952fe3':
  do not lock while writing to a socket (#52) (#54)
  readme: credit @bozaro and @idy (#53)
  Improve cursorData struct unmarshaling speed (#49)
  readme: credit @bozaro (#47)
  Fix GetBSON() method usage (#40)
  readme: credit @feliixx (#46)
  fix golint, go vet and gofmt warnings (#44)
  readme: add missing features / credit
  bson.Unmarshal returns time in UTC (#42)
  Introduce constants for BSON element types (#41)
  Test against MongoDB 3.4.x (#35)
  Add collation option to collection.Create() (#37)
  Don't panic on indexed int64 fields (#23)
  readme: credit @feliixx in the README (#36)
  add method CreateView() (#33)
  Update README to add appName (#32)
  send metadata during handshake (#28)
  readme: credit @feliixx for #25 (#26)
  add DropAllIndexes() method (#25)

# Conflicts:
#	README.md
#	bson/decode.go
@domodwyer domodwyer mentioned this pull request Nov 3, 2017
Merged
domodwyer added a commit that referenced this pull request Nov 6, 2017
@domodwyer
Merge Development (#57)
5be15cc 
* add DropAllIndexes() method (#25)

Create a new method to drop all the indexes of a collection
in a single call

* readme: credit @feliixx for #25 (#26)

* send metadata during handshake (#28)

fix [#484](https://github.com/go-mgo/mgo/issues/484)

Annotate connections with metadata provided by the
connecting client.

informations send:

{
 "aplication": {         // optional
   "name": "myAppName"
 }
 "driver": {
    "name": "mgo",
    "version": "v2"
  },
  "os": {
    "type": runtime.GOOS,
    "architecture": runtime.GOARCH
  }
}

to set "application.name", add `appname` param in options
of string connection URI,
for example : "mongodb://localhost:27017?appname=myAppName"

* Update README to add appName (#32)

* docs: elaborate on what appName does

* readme: add appName to changes

* add method CreateView() (#33)

Fix #30.

Thanks to @feliixx for the time and effort.

* readme: credit @feliixx in the README (#36)

* Don't panic on indexed int64 fields (#23)

* Stop all db instances after tests (go-mgo#462)

If all tests pass, the builds for mongo earlier than 2.6 are still failing.
Running a clean up fixes the issue.

* fixing int64 type failing when getting indexes and trying to type them

* requested changes relating to case statement and panic

* Update README.md to credit @mapete94.

* tests: ensure indexed int64 fields do not cause a panic in Indexes()

See:
* #23
* https://github.com/go-mgo/mgo/issues/475
* go-mgo#476

* Add collation option to collection.Create() (#37)

- Allow specifying the default collation for the collection when creating it.
- Add some documentation to query.Collation() method.

fix #29

* Test against MongoDB 3.4.x (#35)

* test against MongoDB 3.4.x

* tests: use listIndexes to assert index state for 3.4+

* make test pass against v3.4.x

  - skip `TestViewWithCollation` because of SERVER-31049,
    cf: https://jira.mongodb.org/browse/SERVER-31049

  - add versionAtLeast() method in init.js script to better
    detect server version

fixes #31

* Introduce constants for BSON element types (#41)

* bson.Unmarshal returns time in UTC (#42)

* readme: add missing features / credit

* Adds missing collation feature description (by @feliixx).
* Adds missing 3.4 tests description (by @feliixx).
* Adds BSON constants description (by @bozaro).
* Adds UTC time.Time unmarshalling (by @gazoon).

* fix golint, go vet and gofmt warnings (#44)

Fixes #43

* readme: credit @feliixx (#46)

* Fix GetBSON() method usage (#40)

* Fix GetBSON() method usage

Original issue
---

You can't use type with custom GetBSON() method mixed with structure field type and structure field reference type.

For example, you can't create custom GetBSON() for Bar type:

```
struct Foo {
	a  Bar
	b *Bar
}
```

Type implementation (`func (t Bar) GetBSON()` ) would crash on `Foo.b = nil` value encoding.

Reference implementation (`func (t *Bar) GetBSON()` ) would not call on `Foo.a` value encoding.

After this change
---

For type implementation  `func (t Bar) GetBSON()` would not call on `Foo.b = nil` value encoding.
In this case `nil` value would be seariazied as `nil` BSON value.

For reference implementation `func (t *Bar) GetBSON()` would call even on `Foo.a` value encoding.

* Minor refactoring

* readme: credit @bozaro (#47)

* Improve cursorData struct unmarshaling speed (#49)

This change remove full BSON decoding on:

 - parsing to `bson.Raw` and `bson.DocElem` fields;
 - skipping unused BSON fields.

* readme: credit @bozaro and @idy (#53)

* readme: credit @bozaro and @idy

* readme: add @idy to contributor list

* do not lock while writing to a socket (#52) (#54)

fix #51
@domodwyer domodwyer mentioned this pull request Nov 17, 2017
Closed
domodwyer added a commit that referenced this pull request Dec 12, 2017
@domodwyer
Merge branch 'master' into development
1ac9b5d 
* master:
  Merge Development (#57)
  Revert "do not lock while writing to a socket (#52)"
  do not lock while writing to a socket (#52)
  Merge Development (#48)
  Merge development (#39)

# Conflicts:
#	.travis.yml
#	cluster_test.go
#	harness/daemons/.env
#	harness/mongojs/init.js
#	session.go
#	session_internal_test.go
#	session_test.go
@domodwyer domodwyer mentioned this pull request Dec 29, 2017
Closed
libi pushed a commit to libi/mgo that referenced this pull request Dec 1, 2022
@idy @domodwyer
do not lock while writing to a socket (globalsign#52)
de32dd0 
fix globalsign#51
libi pushed a commit to libi/mgo that referenced this pull request Dec 1, 2022
@domodwyer
Revert "do not lock while writing to a socket (globalsign#52)"
9b47735 
This reverts commit 95448ed.
libi pushed a commit to libi/mgo that referenced this pull request Dec 1, 2022
@domodwyer
Merge Development (globalsign#57)
a854aab 
* add DropAllIndexes() method (globalsign#25)

Create a new method to drop all the indexes of a collection
in a single call

* readme: credit @feliixx for globalsign#25 (globalsign#26)

* send metadata during handshake (globalsign#28)

fix [#484](https://github.com/go-mgo/mgo/issues/484)

Annotate connections with metadata provided by the
connecting client.

informations send:

{
 "aplication": {         // optional
   "name": "myAppName"
 }
 "driver": {
    "name": "mgo",
    "version": "v2"
  },
  "os": {
    "type": runtime.GOOS,
    "architecture": runtime.GOARCH
  }
}

to set "application.name", add `appname` param in options
of string connection URI,
for example : "mongodb://localhost:27017?appname=myAppName"

* Update README to add appName (globalsign#32)

* docs: elaborate on what appName does

* readme: add appName to changes

* add method CreateView() (globalsign#33)

Fix globalsign#30.

Thanks to @feliixx for the time and effort.

* readme: credit @feliixx in the README (globalsign#36)

* Don't panic on indexed int64 fields (globalsign#23)

* Stop all db instances after tests (go-mgo#462)

If all tests pass, the builds for mongo earlier than 2.6 are still failing.
Running a clean up fixes the issue.

* fixing int64 type failing when getting indexes and trying to type them

* requested changes relating to case statement and panic

* Update README.md to credit @mapete94.

* tests: ensure indexed int64 fields do not cause a panic in Indexes()

See:
* globalsign#23
* https://github.com/go-mgo/mgo/issues/475
* go-mgo#476

* Add collation option to collection.Create() (globalsign#37)

- Allow specifying the default collation for the collection when creating it.
- Add some documentation to query.Collation() method.

fix globalsign#29

* Test against MongoDB 3.4.x (globalsign#35)

* test against MongoDB 3.4.x

* tests: use listIndexes to assert index state for 3.4+

* make test pass against v3.4.x

  - skip `TestViewWithCollation` because of SERVER-31049,
    cf: https://jira.mongodb.org/browse/SERVER-31049

  - add versionAtLeast() method in init.js script to better
    detect server version

fixes globalsign#31

* Introduce constants for BSON element types (globalsign#41)

* bson.Unmarshal returns time in UTC (globalsign#42)

* readme: add missing features / credit

* Adds missing collation feature description (by @feliixx).
* Adds missing 3.4 tests description (by @feliixx).
* Adds BSON constants description (by @bozaro).
* Adds UTC time.Time unmarshalling (by @gazoon).

* fix golint, go vet and gofmt warnings (globalsign#44)

Fixes globalsign#43

* readme: credit @feliixx (globalsign#46)

* Fix GetBSON() method usage (globalsign#40)

* Fix GetBSON() method usage

Original issue
---

You can't use type with custom GetBSON() method mixed with structure field type and structure field reference type.

For example, you can't create custom GetBSON() for Bar type:

```
struct Foo {
	a  Bar
	b *Bar
}
```

Type implementation (`func (t Bar) GetBSON()` ) would crash on `Foo.b = nil` value encoding.

Reference implementation (`func (t *Bar) GetBSON()` ) would not call on `Foo.a` value encoding.

After this change
---

For type implementation  `func (t Bar) GetBSON()` would not call on `Foo.b = nil` value encoding.
In this case `nil` value would be seariazied as `nil` BSON value.

For reference implementation `func (t *Bar) GetBSON()` would call even on `Foo.a` value encoding.

* Minor refactoring

* readme: credit @bozaro (globalsign#47)

* Improve cursorData struct unmarshaling speed (globalsign#49)

This change remove full BSON decoding on:

 - parsing to `bson.Raw` and `bson.DocElem` fields;
 - skipping unused BSON fields.

* readme: credit @bozaro and @idy (globalsign#53)

* readme: credit @bozaro and @idy

* readme: add @idy to contributor list

* do not lock while writing to a socket (globalsign#52) (globalsign#54)

fix globalsign#51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@domodwyer domodwyer domodwyer approved these changes

@szank szank Awaiting requested review from szank

@brknstrngz brknstrngz Awaiting requested review from brknstrngz

@tadukurow tadukurow Awaiting requested review from tadukurow

Assignees

@idy idy

@domodwyer domodwyer

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@idy @domodwyer