Skip to content

fix fcntl usage when set socket nonblock#69

Merged
MOON-CLJ merged 1 commit intodouban:masterfrom
MOON-CLJ:fix_fcntl
Feb 28, 2018
Merged

fix fcntl usage when set socket nonblock#69
MOON-CLJ merged 1 commit intodouban:masterfrom
MOON-CLJ:fix_fcntl

Conversation

@MOON-CLJ
Copy link
Contributor

@MOON-CLJ MOON-CLJ commented Jan 18, 2018

cc @everpcpc

from fcntl man page

       EINTR  For F_SETLKW, the command was interrupted by a signal; see
       signal(7).  For F_GETLK and F_SETLK, the command was interrupted
       by a signal  before  the
       lock was checked or acquired.  Most likely when
       locking a remote file (e.g., locking over NFS), but
       can sometimes happen locally.

       EINVAL For F_DUPFD, arg is negative or is
       greater than the maximum allowable value.
       For F_SETSIG, arg is not an allowable signal
       number.

so i dont think cmd F_GETFL & F_SETFL will encounter EINTR or EAGAIN,
so just move the do while loop will do the same thing as before.
i see redis source
code,https://github.com/antirez/redis/blob/565e139a5631a777254e222d1c50ea6d696e1a8e/src/anet.c#L77

it's just like

    if (fcntl(fd, F_SETFL, flags) == -1) {
        anetSetError(err, "fcntl(F_SETFL,O_NONBLOCK): %s", strerror(errno));
        return ANET_ERR;
    }

so i think it's safe to do so。

this pr include some other minor fix.

  • str_port's init args from "\0" to "", i think "" just means "\0"
    for c char array literal.

  • max_timeout 6 to 3,i think 6 is too much, for example
    m_connectTimeout is 300ms in douban's mc config.

pollfds[0].fd = fd;
pollfds[0].events = POLLOUT;
int max_timeout = 6;
int max_timeout = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mckelvin no particular reason,just think 6 as ratio is large。

Copy link
Contributor Author

@MOON-CLJ MOON-CLJ Jan 23, 2018

Choose a reason for hiding this comment

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

i will not insist if you think it's not okay。

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to keep it unchanged. If you think it's essential, I think you should also bump the version later to make users noticed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mckelvin okay,i'll do it later if i think it's necessary after more observations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mckelvin updated

from fcntl man page

```
       EINTR  For F_SETLKW, the command was interrupted by a signal; see
       signal(7).  For F_GETLK and F_SETLK, the command was interrupted
       by a signal  before  the
       lock was checked or acquired.  Most likely when
       locking a remote file (e.g., locking over NFS), but
       can sometimes happen locally.

       EINVAL For F_DUPFD, arg is negative or is
       greater than the maximum allowable value.
       For F_SETSIG, arg is not an allowable signal
       number.
```

so i dont think cmd F_GETFL & F_SETFL will encounter EINTR or EAGAIN,
so just move the do while loop will do the same thing as before.
i see redis source
code,https://github.com/antirez/redis/blob/565e139a5631a777254e222d1c50ea6d696e1a8e/src/anet.c#L77

it's just like

```
    if (fcntl(fd, F_SETFL, flags) == -1) {
        anetSetError(err, "fcntl(F_SETFL,O_NONBLOCK): %s", strerror(errno));
        return ANET_ERR;
    }
```

so i think it's safe to do so。

this pr include some other minor fix.

 - str_port's init args from "\0" to "", i think "" just means "\0"
   for c char array literal.

 - max_timeout 6 to 3,i think 6 is too much, for example
   m_connectTimeout is 300ms in douban's mc config.

Conflicts:
	misc/.cppcheck-supp
@MOON-CLJ MOON-CLJ merged commit 4ff6684 into douban:master Feb 28, 2018
tclh123 added a commit that referenced this pull request Feb 18, 2019
ChangeLog:

- use pickle protocol version 2 (#52)
- fix rvalue reference (#53)
- Fix tests (#55)
- golibmc_test: Fix fragile test case (#56)
- Use connection pool in golibmc(again) (#57)
- golibmc Quit ret err (#59)
- cmake: Incorporate gtest in a standard way (#58)
- Rename refactor (#60)
- sync rapidjson/itoa.h upstream fix (#65)
- split cppcheck (#67)
- split FSM_GET_BYTES_CAS (#68)
- Avoid noisy recv_err when broadcast quit (#73)
- Try to support updating some servers without affecting others (#74)
- make normalize_key cpdef & add warnings for unpickle unmarshal fail (#66)
- fix fcntl usage when set socket nonblock (#69)
- minor fix (#61)
- add condition log macro (#77)
- Define and use notWaitForRetryTimeout (#82)
- upgrade travis to gcc 7 (#91)
- add func errCodeToString (#79)
- Introduce a reconnect mechanism in waitPoll (#88)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants